[FFmpeg-devel] BUG in mpeg video decoding? ...

[Michael Niedermayer]

On Mon, Jun 08, 2009 at 11:15:15AM +0200, Luca Abeni wrote:
> Hi Michael,
> 
> On Mon, 2009-06-08 at 01:28 +0200, Michael Niedermayer wrote:
> [...]
> > > How to reproduce: just generate a "test.mpg" file with output_example,
> > > add something like "fprintf(stderr, "Got PTS %Ld\n", picture.pts);"
> > > after avcodec_decode_video2() in ffmpeg.c, and run "ffmpeg -i test.mpg
> > > t1.mpg".
> > > 
> > > Is this a bug? Or did I misunderstand something? If it is a bug, I am
> > > willing to investigate it... Just point me to the code I should look at
> > > (I checked libavcodec/mpeg12.c and libavcodec/mpegvideo.c, and I did not
> > > find any reference to "pts"...).
> > 
> > maybe a missing call to avcodec_get_frame_defaults() / avcidec_alloc_frame()
> > somewhere
> 
> So, the decoder is responsible for calling avcodec_get_frame_defaults()
> or similar?
> I ask because I do not know this stuff too much, and I see that only few
> decoders call avcodec_get_frame_defaults() or avcodec_alloc_frame()...
> I see that avcodec_get_frame_defaults() is called by ffmpeg before
> calling avcodec_decode_video2(), but then the MPEG decoder overwrites
> the pts value with a 0.
> 
> Anyway, I did some investigation and tests, and it seems to me that the
> attached patch fixes the issue (with it applied, the mpeg decoder
> returns AV_NOPTS_VALUE as a pts) without breaking anything (I hope... I
> ran some tests with other decoders, and I do not see regressions).
> 
> I am mostly ignorant about these issues, so the patch might be
> completely wrong... It just seems to work, but I am not sure if this is
> the correct approach. (so, this is probably not meant to be applied, I
> send it to get some feedback and see if this is the correct approach or
> if I am misunderstanding something - BTW, I had no chance to run "make
> test" yet... I'll do it this afternoon).
> If it is wrong or must be improved in some way, let me know and I'll
> work on it.
> 
> > its simply a matter o looking at te get_buffer call and where the frame
> > is then finally output, somewhere there or between pts is likely lost
> 
> If my understanding of the code is correct, the mpeg{1,2} decoder is
> just not setting the pts value. The mpeg4 decoder is setting it, so I
> get meaningful pts values. I tried to add a call to
> avcodec_get_frame_defaults() before alloc_frame_buffer() in mpegvideo.c
> (which calls get_buffer()), and this fixed the mpeg{1,2} case... But
> broke mpeg4 decoding, because it forced the mpeg4 decoder to always
> return AV_NOPTS_VALUE as a pts (I think the pts is set before calling
> alloc_frame_buffer())...
> The attached patch is a little bit more complex, but does not present
> this problem.
> 
> 
> 			Thanks,
> 				Luca

>  mpegvideo.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> a7f886697c278bfae0b1501bb5e0e5239e4abcb7  maybe_fix_decoder_pts-1.diff
> Index: libavcodec/mpegvideo.c
> ===================================================================
> --- libavcodec/mpegvideo.c	(revision 19128)
> +++ libavcodec/mpegvideo.c	(working copy)
> @@ -819,14 +819,23 @@
>  
>      if(shared){
>          for(i=0; i<MAX_PICTURE_COUNT; i++){
> -            if(s->picture[i].data[0]==NULL && s->picture[i].type==0) return i;
> +            if(s->picture[i].data[0]==NULL && s->picture[i].type==0) {
> +                avcodec_get_frame_defaults((AVFrame *)&s->picture[i]);
> +                return i;
> +            }
>          }

can it also be done during codec init?
if so that would be preferable

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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-devel/attachments/20090608/add0dd09/attachment.pgp>