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

[Bartlomiej Wolowiec]

On niedziela, 13 kwietnia 2008, Michael Niedermayer wrote:
> On Sat, Apr 12, 2008 at 11:12:35PM +0200, Bartlomiej Wolowiec wrote:
> > On sobota, 12 kwietnia 2008, Michael Niedermayer wrote:
>
> [...]
>
> > > > [...]
> > > >
> > > > --
> > > > 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?
>
> You claim that you found the end of a frame at position 0 while in reality
> you havnt found anything. That is you MUST pass END_NOT_FOUND.

Ok, I'll add corrected version with frame_in_buffer variable.

> > 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
>
> no, ff_combine_frame() contains code to handle EOF you can grep for 'EOF'
> to see it, you can just add a av_log() to see that it does return a
> different value at EOF
>
> > 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;
> > [...]
>
> no
> your code must contain something like
>
> if (ff_combine_frame(...) < 0) {
>
> or
> blah= ff_combine_frame(...);
> if(blah<0)
>
> or if(blah >= 0)
>
> [...]

Hmm... I still don't understand what exactly it's about:
ff_combine_frame(pc, i, &buf, &buf_size) // if i!=END_NOT_FOUND this line 
always returns 0
ff_combine_frame(pc, END_NOT_FOUND, &buf, &buf_size) // this line returns 0 
only when buf_size=0

So, maybe you prefer such a solution?

(instead of ff_combine_frame at the end of function)
if(ff_combine_frame(pc, END_NOT_FOUND, &buf, &buf_size)>=0 
&& !s->remaining_size){
? ? if(buf_size){
? ? ? ? //here goto update codec info and output frame
? ? }
}


-- 
Bartlomiej Wolowiec