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

[Michael Niedermayer]

On Sun, Apr 13, 2008 at 03:45:13PM +0200, Bartlomiej Wolowiec wrote:
> 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
> ? ? }
> }

uhm, no

the documentation of ff_combine_frame() says:
/**
 * combines the (truncated) bitstream to a complete frame
 * @returns -1 if no complete frame could be created, AVERROR(ENOMEM) if there was a memory allocation error
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 */

the return value tells you if you have a complete frame.
grep for ff_combine_frame() you will find code like:

        next= ff_mpeg1_find_frame_end(pc, buf, buf_size);

        if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
            *poutbuf = NULL;
            *poutbuf_size = 0;
            return buf_size;
        }

...
    /* we have a full frame : we just parse the first few MPEG headers
       to have the full timing information. The time take by this
       function should be negligible for uncorrupted streams */
    mpegvideo_extract_headers(s, avctx, buf, buf_size);
...
    *poutbuf = buf;
    *poutbuf_size = buf_size;
    return next;


I dont understand what can be unclear on this, you call ff_combine_frame()
it checks if it has a complete frame and tells you if you so by its return
value. And you use this and return the appropriate case.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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-cvslog/attachments/20080413/506a49ea/attachment.pgp>