[FFmpeg-devel] [PATCH] Demuxer for Leitch/Harris' VR native stream format (LXF)

[Måns Rullgård]

Tomas H?rdin <tomas.hardin at codemill.se> writes:

>> > > > +//returns number of bits set in value
>> > > > +static int num_set_bits(uint32_t value) {
>> > > > +    int ret;
>> > > > +
>> > > > +    for(ret = 0; value; ret += (value & 1), value >>= 1);
>> > > > +
>> > > > +    return ret;
>> > > > +}
>> > > 
>> > > if we dont have a population count function yet, than one should be added
>> > > to some header in libavutil
>> > 
>> > I couldn't find one. That probably belongs in its own thread though.
>> > 
>> > Which files would such a function belong in - intmath.h/c, common.h or
>> > somewhere else? Also, which name would be best: ff_count_bits(),
>> > av_count_bits() or something else?
>> 
>> av_popcount()
>> would be similar to gccs __builtin_popcount()
>
> OK. I attached popcount.patch which adds such a function to common.h.
> Also bumped minor of lavu. The implementation uses a 16-byte LUT and
> therefore counts four bits at a time. I suspect there are better
> solutions though. I did verify that it returns exactly the same number
> the other implementation does for all 2^32 possible input values.

I can't think of a better generic solution off the top of my head.

> A compiler version check could use the builtin, but I'll leave that to
> someone more knowledgable.

Added to my ever-growing to do list.

>> [...]
>> > +//reads and checksums packet header. returns format and size of payload
>> > +static int get_packet_header(AVFormatContext *s, unsigned char *header,
>> > +                             uint32_t *format)
>> > +{
>> > +    LXFDemuxContext *lxf = s->priv_data;
>> > +    ByteIOContext   *pb  = s->pb;
>> > +    int size, track_size, samples;
>> > +    AVStream *st;
>> > +
>> > +    if (get_buffer(pb, header, LXF_PACKET_HEADER_SIZE) != LXF_PACKET_HEADER_SIZE)
>> > +        return AVERROR(EIO);
>> > +
>> > +    if (memcmp(header, LXF_IDENT, LXF_IDENT_LENGTH)) {
>> > +        av_log(s, AV_LOG_ERROR, "packet ident mismatch - out of sync?\n");
>> > +        return -1;
>> > +    }
>> > +
>> > +    if (check_checksum(header))
>> > +        av_log(s, AV_LOG_ERROR, "checksum error\n");
>> > +
>> > +    *format = AV_RL32(&header[32]);
>> > +    size    = AV_RL32(&header[36]);
>> > +
>> > +    //type
>> > +    switch (AV_RL32(&header[16])) {
>> > +    case 0:
>> > +        //video
>> > +        //skip VBI data and metadata
>> > +        url_fskip(pb, AV_RL32(&header[44]) + AV_RL32(&header[52]));
>> 
>> cant this lead to a backward seek and thus infinite loop ?
>
> Yes, assuming AV_RL32() returns signed. Fixed by casting to uint32_t and
> splitting up into two skips.

AV_RL32() returns an unsigned value.

> diff --git a/libavutil/common.h b/libavutil/common.h
> index d054f87..824bd8c 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h

This belongs in intmath.h IMO.

> @@ -192,6 +192,22 @@ static inline av_const int av_ceil_log2_c(int x)
>      return av_log2((x - 1) << 1);
>  }
>  
> +/**
> + * Count number of bits set to one in x
> + * @param x value to count bits of
> + * @return the number of bits set to one in x
> + */
> +static inline av_const int av_popcount_c(uint32_t x)
> +{
> +    int i, ret = 0;
> +    const uint8_t tab[16] = {0,1,1,2,1,2,2,3,1,2,2,3,2,3,3,4};

Should be static.

> +    for(i = 0; i < 32; i += 4)
> +        ret += tab[(x >> i) & 0xF];

I'd unroll this by hand rather than trusting it to the compiler.

-- 
M?ns Rullg?rd
mans at mansr.com