[FFmpeg-devel] [RFC] Error concealment for B-frames/fixing issue 824

Michael Niedermayer michaelni
Fri May 15 00:57:00 CEST 2009


On Thu, May 14, 2009 at 01:42:33PM -0700, Baptiste Coudurier wrote:
> Baptiste Coudurier wrote:
> > On 5/5/2009 8:34 PM, Michael Niedermayer wrote:
> >> On Tue, May 05, 2009 at 07:56:39PM -0700, Baptiste Coudurier wrote:
> >>> On 5/5/2009 5:17 PM, Michael Niedermayer wrote:
> >>>> On Tue, May 05, 2009 at 02:08:33PM -0700, Baptiste Coudurier wrote:
> >>>>> Baptiste Coudurier wrote:
> >>>>>> On Fri, Apr 10, 2009 at 06:50:08PM -0700, Baptiste Coudurier wrote:
> >>>>>>> Hi Reimar,
> >>>>>>>
> >>>>>>> On 4/9/2009 3:13 PM, Reimar D?ffinger wrote:
> >>>>>>>> On Thu, Apr 09, 2009 at 10:52:32PM +0200, Michael Niedermayer wrote:
> >>>>>>>>> there are 2 very seperate things
> >>>>>>>>> 1. errors
> >>>>>>>>> 2. b frames without reference frames after seeking
> >>>>>>>>>
> >>>>>>>>> normal users dont want to see randomly trashed frames after seeking
> >>>>>>>>> in undamged files
> >>>>>>>> I think I agree... Is there a counter somewhere that could be used to
> >>>>>>>> find out which frame in the GOP we are at?
> >>>>>>> Yes temp_ref in pic structure.
> >>>>>>>
> >>>>>>>> Some options I can think of:
> >>>>>>>> 1) change code to not skip a B-frame if it is the second frame in a closed GOP
> >>>>>>>> 2) only skip B-frames if they directly follow the first I-frame of a non-closed GOP
> >>>>>>>> 3) (without really knowing what "broken link" means) only skip B-frames if "broken
> >>>>>>>> link" is set
> >>>>>>> From what I understand, broken link explicitly state that the 2 next b
> >>>>>>> frames cannot be decoded because the stream was cut at the preceding I
> >>>>>>> frame, therefore the original reference I frame is no more available.
> >>>>>>>
> >>>>>>> And I think your strategy should be ok :)
> >>>>>> Here is another attempt.
> >>>>>>
> >>>>>> It will decodes them only is closed gop is set.
> >>>>>> I got a sample with open gop and B frames before the I frame, it
> >>>>>> works, also, decoding them anyway does not crash when dummy picture is
> >>>>>> allocated some block are gray.
> >>>>>>
> >>>>>> Patch attached.
> >>>>>>
> >>>>> New patch attached, more correct.
> >>>> looks ok
> >>> I've tested it more to be sure and I've encountered this problem during
> >>> seeking with ffplay:
> >>>
> >>> [mpeg2video @ 0x223f6a0]pic->data[0]!=NULL in avcodec_default_get_buffer
> >>> [mpeg2video @ 0x223f6a0]get_buffer() failed (-1 2147483647 8 0x3269950)
> >>> [mpeg2video @ 0x223f6a0]pic->data[0]!=NULL in avcodec_default_get_buffer
> >>> [mpeg2video @ 0x223f6a0]get_buffer() failed (-1 2147483647 8 0x3269950)
> >>> Seek to 63% ( 0:01:23) of total duration ( 0:02:12)
> >>> [mpeg2video @ 0x223f6a0]pic->data[0]!=NULL in avcodec_default_get_buffer
> >>> [mpeg2video @ 0x223f6a0]get_buffer() failed (-1 2147483647 8 0x3140e30)
> >>> [mpeg2video @ 0x223f6a0]pic->data[0]!=NULL in avcodec_default_get_buffer
> >>> [mpeg2video @ 0x223f6a0]get_buffer() failed (-1 2147483647 8 0x3140e30)
> >>>
> >>> So I guess allocating last_picture this way was not correct.

likely, yes


> >>>
> >>> I think copying next picture to last picture is better then.
> >> i dont mind the picture being copied (pixel wise) but having
> >> s2->last_picture_ptr == s2->next_picture_ptr
> >> gives me a bad feeling, this is not a conditiom that could be true otherwise
> > 
> > Ok. It seems copying and not setting last_picture_ptr to
> > next_picture_ptr does not work and crashes. So I feel going to back to
> > first solution and finding where to correctly free the pic. Any hint ?
> > 
> 

[...]

> int MPV_frame_start(MpegEncContext *s, AVCodecContext *avctx)
> {
>     int i;
>     AVFrame *pic;
>     s->mb_skipped = 0;
> 
>     assert(s->last_picture_ptr==NULL || s->out_format != FMT_H264 ||
> s->codec_id == CODEC_ID_SVQ3);
> 
>     /* mark&release old frames */
>     if (s->pict_type != FF_B_TYPE && s->last_picture_ptr &&
> s->last_picture_ptr != s->next_picture_ptr &&
> s->last_picture_ptr->data[0]) {
>       if(s->out_format != FMT_H264 || s->codec_id == CODEC_ID_SVQ3){
>           free_frame_buffer(s, s->last_picture_ptr);
> 
> Does this check implies that in some situation, last_picture_ptr might
> be next_picture_ptr ?

yes but not for mpeg1/2 nor any other codec supporting B frames _IIRC_


> In this case, is it so wrong to set it so for the
> closed gop case ?

putting the picture buffers into a state that was not possible previously
requires the one doing it to fully understand the code in question.
Someone fully understanding the code should have no problem simply allocating
a new picture in last_picture* instead of setting it to next_picture_ptr.

summary being, i am not against setting them equal, what iam against is this
kind of
first try, hmm it crashes, no faint clue why, lets try something else random,
ahh doesnt crash, lets submit and let michael figure out if its ok, and if
it breaks something let michael fix it later ...

thus given these circumtances i prefer(ed) that the buffers are kept in
the normal state with next != last

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20090515/03920625/attachment.pgp>



More information about the ffmpeg-devel mailing list