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

[Michael Niedermayer]

On Thu, May 14, 2009 at 04:10:30PM -0700, Baptiste Coudurier wrote:
> Hi Michael,
> 
> Michael Niedermayer wrote:
> > 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_
> 
> All right, thanks for the information.
> 
> >> 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 ...
> 
> Yes, of course, I did not imply this, sorry if I appeared that way.
> 
> > thus given these circumtances i prefer(ed) that the buffers are kept in
> > the normal state with next != last
> 
> No problem, I'm just now trying to better understand the situation
> according to the code, like you are suggesting.
> 
> Situation is: if alloc_frame is used, tracking the buffers and free them
> is needed.

what about ff_find_unused_picture() / alloc_picture()
?

> If next_picture_ptr is copied, but last_picture_ptr not set to the same
> value, it avoids allocation. However it seems the frame will be freed in
> free_frame_buffer later while pic->type will be FF_BUFFER_TYPE_COPY.

i dont think it would be FF_BUFFER_TYPE_COPY ?
last_picture may but last_picture_ptr should not i think but its all a
while ago since i wrote that code
that last/next should always be copies of the _ptr versions, the _ptr
should not point to last/next

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/89a8e908/attachment.pgp>