[FFmpeg-soc] ac3 parser (adding parse_header)

[Michael Niedermayer]

On Fri, May 02, 2008 at 06:57:19PM +0200, Bartlomiej Wolowiec wrote:
> log:
> Adding parse_header in aac_ac3_parser and correct read of number of channels 
> in eac3.
> 
> can you check the patch, please?
[...]
> Index: libavcodec/aac_ac3_parser.h
> ===================================================================
> --- libavcodec/aac_ac3_parser.h	(wersja 13030)
> +++ libavcodec/aac_ac3_parser.h	(kopia robocza)
> @@ -31,6 +31,8 @@
>      int header_size;
>      int (*sync)(uint64_t state, struct AACAC3ParseContext *hdr_info,
>              int *need_next_header, int *new_frame_start);

> +    int (*parse_header)(const uint8_t *buf, int buf_size,
> +            struct AACAC3ParseContext *hdr_info);

I would prefer AACAC3ParseContext to be the first argument, or more genrically
that the context is always the first argument.


[...]
> +static int ac3_parse_header(const uint8_t *buf, int buf_size,
> +        AACAC3ParseContext *hdr_info){
> +    uint16_t bitmask=0;
> +    AC3HeaderInfo hdr;
> +    GetBitContext gbc;
> +    int i=0;
> +
> +    hdr_info->bit_rate = 0;
> +    do{
> +        init_get_bits(&gbc, buf+i, buf_size-i);
> +        if(ff_ac3_parse_header_full(&gbc, &hdr)<0 || !hdr.frame_size)

At the very least, the names are very badly choosen.

ff_ac3_parse_header()
ff_ac3_parse_header_full()
ac3_parse_header()

having a function with the name X() repeatly call a function with name
X_full() is definitly not sane.

The functions (if 3 are needed at all) should be renamed so its obvious
from the names what they do.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080502/f89e444f/attachment.pgp>