[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