[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 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.
>
> [...]

Hi, I think that I got lost somewhere, because I can't see the main goal of 
this... 
Generally, I hope that you thought about a? solution like the solution in 
attachment (or something in this direction).

-- 
Bartlomiej Wolowiec
-------------- next part --------------
A non-text attachment was scrubbed...
Name: parser.patch
Type: text/x-diff
Size: 7787 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20080418/f0017a16/attachment.patch>