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

Michael Niedermayer michaelni
Fri Apr 18 01:13:30 CEST 2008


On Fri, Apr 18, 2008 at 12:10:35AM +0200, Bartlomiej Wolowiec wrote:
> 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... 

The main goal is to pass values which represent the truth and check the
return value of ff_combine_frame()
what you do is a little like malloc(7) and use 8 bytes because malloc
happens to round it up internally.


> Generally, I hope that you thought about a? solution like the solution in 
> attachment (or something in this direction).

i wanted you to add checks for the return value of ff_combine_frame() and
to pass a truthfull parameter which says if you found a frame boundary.
You still do not do this, you still explicitly check for EOF and then
provide ff_combine_frame() with a fake "i found a startcode here at 0".
If that was true it would also be if buf_size>0 but in that case its not
so it cannot be true if buf_size==0.

So please remove the buf_size==0 check.

Also the code was working without such checks already, it just failed at
EOF when ff_combine_frame() told you that it had a frame but your code
continued as if ff_combine_frame() had none.

besides this, the code now is completely reordered and its somewhat hard
to check the correctness of the new code. at least the frame_in_buffer
code is buggy now though.

Anyway i really dont care if the code uses a while() or the new variant
which is turned inside out with a goto.


[...]

> @@ -24,40 +24,60 @@
>  #include "aac_ac3_parser.h"
>  
>  int ff_aac_ac3_parse(AVCodecParserContext *s1,
> -                     AVCodecContext *avctx,
> -                     const uint8_t **poutbuf, int *poutbuf_size,
> -                     const uint8_t *buf, int buf_size)
> +        AVCodecContext *avctx,
> +        const uint8_t **poutbuf, int *poutbuf_size,
> +        const uint8_t *buf, int buf_size)

cosmetic

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080418/53cab776/attachment.pgp>



More information about the ffmpeg-cvslog mailing list