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

[Michael Niedermayer]

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.


> 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)

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/86785070/attachment.pgp>