[FFmpeg-devel] [RFC] Error concealment for B-frames/fixing issue 824
Baptiste Coudurier
baptiste.coudurier
Fri May 15 07:40:41 CEST 2009
On Fri, May 15, 2009 at 06:03:54AM +0200, Michael Niedermayer wrote:
> On Thu, May 14, 2009 at 07:04:39PM -0700, Baptiste Coudurier wrote:
> > On Fri, May 15, 2009 at 03:55:28AM +0200, Michael Niedermayer wrote:
> > > On Thu, May 14, 2009 at 06:03:54PM -0700, Baptiste Coudurier wrote:
> > > > Michael Niedermayer wrote:
> > > > > 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()
> > > > > ?
> > > >
> > > > Ahhhhh, here it is :)
> > > >
> > > > >> 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
> > > >
> > > > Yes, it seems allocating &s2->last_picture directly was very wrong.
> > > >
> > > > Updated patch attached.
> > >
> > > isnt that missing code to reset closed_gop in a few cases like
> > > seeking?
> > >
> >
> > Technically the patch attached was missing the diff to mpegvideo.c, so
> > yes :)
> >
> > Here it is.
>
> patch possibly ok given that it works, valgrind says nothing new
> and there arent (more than very few) files out there with these flags
> being wrong.
>
Great, it works and valgrind is quiet. Let's hope there aren't so many
broken encoders in the wild ;)
Thanks for the help, applied.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list