[FFmpeg-soc] extension of ac3 parser

[Justin Ruggles]

Bartlomiej Wolowiec wrote:
> On niedziela, 20 kwietnia 2008, Michael Niedermayer wrote:
>> On Sun, Apr 20, 2008 at 01:30:03AM +0200, Bartlomiej Wolowiec wrote:
>>> I have a question, do the changes in parser, allowing it to correctly
>>> read the number of channels from package of frames, should be commited to
>>> soc repository or prepared to main repository?
>> main repo!
>>
>> [....]
> 
> Ok. I enclose patch
> 

> Index: libavcodec/ac3tab.c
> ===================================================================
> --- libavcodec/ac3tab.c	(wersja 12910)
> +++ libavcodec/ac3tab.c	(kopia robocza)
> @@ -247,3 +247,25 @@
>      1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 3, 3, 3, 3, 3, 3,
>      3, 6, 6, 6, 6, 6, 6, 12, 12, 12, 12, 24, 24, 24, 24, 24
>  };
> +
> +const uint8_t ff_ac3_channels_multiple[16] = {
> +    1,1,0,1,2,2,2,1,
> +    1,2,2,1,1,1,1,1
> +};

document this. it's not immediately obvious what it's used for.

> +/** Custom channel map locations bitmask
> + *  Other channels described in documentation:
> + *      Lc/Rc pair, Lrs/Rrs pair, Ts, Lsd/Rsd pair,
> + *      Lw/Rw pair, Lvh/Rvh pair, Cvh, Reserved, LFE2
> + */
> +enum CustomChannelMapLocation{
> +    AC3_CHMAP_LEFT =            1<<(15-0),
> +    AC3_CHMAP_CENTRE =          1<<(15-1),
> +    AC3_CHMAP_RIGHT =           1<<(15-2),
> +    AC3_CHMAP_LEFT_SURROUND =   1<<(15-3),
> +    AC3_CHMAP_RIGHT_SURROUND =  1<<(15-4),
> +    AC3_CHMAP_CENTRE_SURROUND = 1<<(15-7),
> +    AC3_CHMAP_LFE =             1<<(15-15)
> +};

Use American English (center, not centre)

> @@ -118,6 +123,30 @@
>          hdr->bit_rate = (uint32_t)(8.0 * hdr->frame_size * hdr->sample_rate /
>                          (num_blocks * 256.0));
>          hdr->channels = ff_ac3_channels_tab[hdr->channel_mode] + hdr->lfe_on;
> +
> +        if(read_channel_map){
> +            int i;
> +            skip_bits(&gbc, 5); // skip bitstream id
> +
> +            /* dialog normalization and compression gain are volume control params */

I'd rather just know here that you're skipping them, not what they're
used for since it's not really applicable here.

> +int ff_ac3_parse_channels_info(const uint8_t *buf, int buf_size,
> +        uint16_t *channels_bitmask, int *rep_channels,
> +        int *bit_rate){
> +    AC3HeaderInfo hdr;
> +    int j, i, all_channels=0;
> +    uint16_t bitmask;
> +
> +    *bit_rate = 0;
> +    memset(rep_channels, 0, sizeof(*rep_channels)*16);
> +    bitmask = 0;
> +    j = 0;
> +    do{
> +        if(ff_ac3_parse_header(buf+j, &hdr, 1)<0)
> +            return -1;
> +        bitmask |= hdr.channel_map;
> +
> +        for(i=0; hdr.channel_map; i++, hdr.channel_map>>=1){
> +            if(hdr.channel_map&1){
> +                (rep_channels[i])++;
> +            }
> +        }
> +        *bit_rate += hdr.bit_rate;
> +        j += hdr.frame_size;
> +    }while(j!=buf_size);

It would be safer to do:
}while(j<=buf_size);

Also, you need to check the value of hdr.frame_size to make sure you
don't get stuck in an infinite loop.

> +        if((hdr_info->channels = ff_ac3_parse_channels_info(buf, buf_size,
> +                        &channels_bitmask, rep_channels, &hdr_info->bit_rate))<0)
> +            return -1;
> +    }

I think that this construct is discouraged in FFmpeg. I'd rather see the
value set first, then the comparison.

> +/**
> + * Reads full information about channels in eac3 frames
> + * @param buf[in] Array with frames

It's not really an "array". That would imply that each item is a unique
entity. In this case, it's just a bunch of bytes, so the term is a
little misleading here.


I'm just getting my feet wet again as far as reviews, so I'm sure
Michael will find more things to comment on as well.

Thanks,
Justin