[FFmpeg-devel] [RFC] H.264 error concealment and mpegvideo frame handling

[Jason Garrett-Glaser]

On Sun, Sep 26, 2010 at 5:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Sep 26, 2010 at 05:01:21PM -0700, Jason Garrett-Glaser wrote:
>> On Sun, Sep 26, 2010 at 5:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Sat, Sep 25, 2010 at 08:48:28PM -0700, Jason Garrett-Glaser wrote:
>> >> On Sat, Sep 25, 2010 at 7:57 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> > On Sat, Sep 25, 2010 at 12:18:18AM -0700, Jason Garrett-Glaser wrote:
>> >> >> On Fri, Sep 24, 2010 at 12:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> >> > On Fri, Sep 24, 2010 at 11:50:52AM -0700, Jason Garrett-Glaser wrote:
>> >> >> >> On Fri, Sep 24, 2010 at 10:56 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> >> >> > On Wed, Sep 22, 2010 at 01:29:35AM -0700, Jason Garrett-Glaser wrote:
>> >> >> >> >> ffh264 does not handle the case of missing reference frames correctly.
>> >> >> >> >> ?Suppose we have 16 refs and we just decoded frame num 18. ?Here's our
>> >> >> >> >> reflist:
>> >> >> >> >>
>> >> >> >> >> 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4
>> >> >> >> >>
>> >> >> >> >> Then, we decode frame num 20 (19 went missing):
>> >> >> >> >>
>> >> >> >> >> 4 18 17 16 15 14 13 12 11 10 9 8 7 6 5
>> >> >> >> >>
>> >> >> >> >> This is obviously buggy and wrong. ?So I do this to fix it:
>> >> >> >> >>
>> >> >> >> >> --- libavcodec/h264.c (revision 25148)
>> >> >> >> >> +++ libavcodec/h264.c (working copy)
>> >> >> >> >> @@ -1905,6 +1905,8 @@
>> >> >> >> >> ? ? ? ? ? ? ?s->current_picture_ptr->frame_num= h->prev_frame_num;
>> >> >> >> >> ? ? ? ? ? ? ?ff_generate_sliding_window_mmcos(h);
>> >> >> >> >> ? ? ? ? ? ? ?ff_h264_execute_ref_pic_marking(h, h->mmco, h->mmco_index);
>> >> >> >> >> + ? ? ? ? ? ?ff_copy_picture(h->short_ref[0], h->short_ref[1]);
>> >> >> >> >> + ? ? ? ? ? ?h->short_ref[0]->frame_num = h->prev_frame_num;
>> >> >> >> >> ? ? ? ? ?}
>> >> >> >> >>
>> >> >> >> >> ? ? ? ? ?/* See if we have a decoded first field looking for a pair... */
>> >> >> >> >>
>> >> >> >> >> This gives us:
>> >> >> >> >>
>> >> >> >> >> 18 18 17 16 15 14 13 12 11 10 9 8 7 6 5
>> >> >> >> >
>> >> >> >> > shouldnt that be 19 18 ... ?
>> >> >> >>
>> >> >> >> 19 doesn't exist. ?We want to copy 18 into where 19 would have been,
>> >> >> >> and give it 19's frame number.
>> >> >> >
>> >> >> > thats what i ve meant, yes
>> >> >> >
>> >> >> >
>> >> >> >>
>> >> >> >> > either way ff_copy_picture() is wrong, we should for the entries that are
>> >> >> >> > left after gap handling allocate seperate pictures and fill them (this
>> >> >> >> > filling could then in the future be replaced by code that uses the b frame
>> >> >> >> > direct mode and motion vectors of the surrounding frames)
>> >> >> >>
>> >> >> >> Attached is an extremely hacky and stupid patch that fixes the
>> >> >> >> problem. ?Here's a sample that makes it blatantly obvious:
>> >> >> >> http://www.mediafire.com/?6huh99qemkrp11y . ?This sample has a frame
>> >> >> >> dropped every 5 frames, then has the encoder fix it via reference
>> >> >> >> invalidation.
>> >> >> >>
>> >> >> >> Dark Shikari
>> >> >> >
>> >> >> >> ?h264.c | ? ?8 ++++++++
>> >> >> >> ?1 file changed, 8 insertions(+)
>> >> >> >> 507842c8cfd74067c2725dc2cf32b77f392a0ba9 ?test2.diff
>> >> >> >
>> >> >> > the principle idea looks good
>> >> >> > it should use av_image_copy()
>> >> >> > and feel fre to commit once you think its clean enough
>> >> >>
>> >> >> Do we have to worry at all about emulated edge stuff? ?i.e. do any of
>> >> >> the refs have pixels beyond the edges we should copy?
>> >> >
>> >> > yes, they can have, ive forgotten about that
>> >>
>> >> Possible solutions:
>> >>
>> >> 1. ?Come up with an av_image_copy that copies those pixels?
>> >>
>> >> 2. ?Ignore them, since out of frame MVs are not a big deal compared to
>> >> every other aspect of error concealment.
>> >
>> > iam fine with 2+documentation of this issue, iam also fine with 1
>>
>> Is this correct?
>
> i see nothing wrong about it but there could be a practical issue
> this loop can execute quite often (i saw it) with the right input.
> I remember thinking that in these cases many iterations could probably
> be just skiped without a difference at the end, but i dont think i tried/fixed
> that

That was a worry of mine as well.  Technically, the loop shouldn't
have to run more than 16 times (to clear the reference list) no matter
the magnitude of the frame num gap.

Updated version attached without compiler warnings.

Dark Shikari
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.diff
Type: application/octet-stream
Size: 1482 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100926/83d04b43/attachment.obj>