[FFmpeg-cvslog] r12759 - in trunk/libavcodec: aac_ac3_parser.c?aac_ac3_parser.h aac_parser.c ac3_parser.c

[Bartlomiej Wolowiec]

On sobota, 12 kwietnia 2008, Michael Niedermayer wrote:
> On Sat, Apr 12, 2008 at 12:27:49AM +0200, Bartlomiej Wolowiec wrote:
> > On ?roda, 9 kwietnia 2008, Michael Niedermayer wrote:
> > > I suspect that the best course of action is to revert the changes up to
> > > the point where the regression tests pass again.
> > > Next the rm demuxer must be fixed which will likely change the
> > > regression checksums, which need to be updated after confirming that
> > > the rm demuxer works equally well or better.
> > > And last the parser should be commited again. (but of course only if
> > > regressions pass, if they dont the bugs causing the failure must be
> > > found and fixed similar to the rm demxuer _before_ commiting the
> > > parser)
> > >
> > > [...]
> >
> > This is patch version with all corrections. But if after ac3 frames are
> > eac3 frames, I don't know what to do to have seektest passed?
> >
> > tests/data/a-ac3.rm
> > [...]
> > -ret: 0 st: 0 dts:0.000000 pts:0.000000 pos:271 size:556 flags:1
> > +ret: 0 st: 0 dts:0.000000 pts:0.000000 pos:839 size:556 flags:1
> >  ret: 0 st:-1 ts:2.576668 flags:0
> > -ret: 0 st: 0 dts:2.960000 pts:2.960000 pos:48659 size:558 flags:1
> > +ret: 0 st: 0 dts:2.960000 pts:2.960000 pos:49229 size:558 flags:1
> > [...]
> >
> > tests/data/b-libav.rm:
> > [...]
> > -ret: 0 st: 1 dts:0.975000 pts:0.975000 pos:355101 size:278 flags:1
> > +ret: 0 st: 1 dts:0.975000 pts:0.975000 pos:-1 size:278 flags:1
> >  ret: 0 st: 1 ts:1.471000 flags:1
> > -ret: 0 st: 1 dts:0.975000 pts:0.975000 pos:355101 size:278 flags:1
> > +ret: 0 st: 1 dts:0.975000 pts:0.975000 pos:-1 size:278 flags:1
>
> As you can see the pos value is wrong after your patch, i suspect it
> might need to be reordered similar to pts/dts in av_parser_parse()

I see that there is a problem with shift of position, but unfortunately I 
don't exactly know how to fix it and how pts/dts work...

> > [...]
> >
> > --
> > Bartlomiej Wolowiec
> >
> > Index: libavcodec/aac_ac3_parser.c
> > ===================================================================
> > --- libavcodec/aac_ac3_parser.c	(wersja 12790)
> > +++ libavcodec/aac_ac3_parser.c	(kopia robocza)
> > @@ -29,60 +29,46 @@
> >                       const uint8_t *buf, int buf_size)
> >  {
> >      AACAC3ParseContext *s = s1->priv_data;
> > -    AACAC3FrameFlag frame_flag;
> > -    const uint8_t *buf_ptr;
> > -    int len;
> > +    ParseContext *pc = &s->pc;
> > +    int len, i;
> >
> > -    *poutbuf = NULL;
> > -    *poutbuf_size = 0;
> > +    while(s->remaining_size <= buf_size){
> >
> > +        if(s->remaining_size && !s->need_next_header || !buf_size){
> > +            i= s->remaining_size;
> > +            /* If EOF set remaining_size>0, to finish correctly. */
> >
> > +            s->remaining_size = !buf_size;
>
> this is VERY ugly
>
> you still ignore the return of ff_combine_frame() you just skip the
> correct call, and luckily even with wrong arguments ff_combine_frame()
> works halfway. But then your hack requires another hack, namely setting
> remaining_size to 1 so the previous hack doesnt trigger again
> this is completely unacceptable.

What do you mean writing about?wrong arguments?
In the case of values returned directly, do you think that ff_combine_frame 
can return AVERROR(ENOMEM) ? (Besides that, I examined uses of this function 
in the code and I wonder why it isn't served anywhere - majority omits this 
part of the buffer, where the lack of memory happened). How ENOMEN should be 
served? (can parser signal that an error happened?). In the rest of cases, if 
next >=END_NOT_FOUND then if there is no ENOMEM it will return buffer size, 
else if next=END_NOT_FOUND it will return -1.

Is this a good solution for EOF:

[...]
        if(s->remaining_size && !s->need_next_header || !buf_size){
            i= s->remaining_size;
            s->remaining_size = 0;
            goto output_frame;
        }else{ //we need a header first
[...]
output_frame:
                    ff_combine_frame(pc, i, &buf, &buf_size)
                    if(!buf_size)
                        return 0;
[...]


> > +static int aac_parse_header(const uint8_t *buf, int buf_size,
> > +        AACAC3ParseContext *hdr_info, AVCodecContext *avctx){
> > +
> > +    /* allow downmixing to stereo */
> > +    if(avctx->request_channels > 0 &&
> > +            avctx->request_channels < hdr_info->channels &&
> > +            avctx->request_channels == 2){
> > +        avctx->channels = avctx->request_channels;
> > +    } else {
> > +        avctx->channels = hdr_info->channels;
> > +    }
> > +    avctx->sample_rate = hdr_info->sample_rate;
> > +    avctx->bit_rate = hdr_info->bit_rate;
> > +    avctx->frame_size = hdr_info->samples;
> > +    return 0;
> > +}
>
> [...]
>
> > +static int ac3_parse_header(const uint8_t *buf, int buf_size,
> > +        AACAC3ParseContext *hdr_info, AVCodecContext *avctx){
> > +
> > +    /* allow downmixing to mono or stereo */
> > +    if(avctx->request_channels > 0 &&
> > +            avctx->request_channels < hdr_info->channels &&
> > +            avctx->request_channels <= 2){
> > +        avctx->channels = avctx->request_channels;
> > +    } else {
> > +        avctx->channels = hdr_info->channels;
> > +    }
> > +    avctx->sample_rate = hdr_info->sample_rate;
> > +    avctx->bit_rate = hdr_info->bit_rate;
> > +    avctx->frame_size = hdr_info->samples;
> > +    return 0;
> > +}
>
> duplicate
> and this belongs in a seperate patch

Ok, I will separate it and send patch only when there is eac3 service, so that 
this codes won't be similar.

-- 
Bartlomiej Wolowiec