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

[Baptiste Coudurier]

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

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org