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

[Jason Garrett-Glaser]

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?

Dark Shikari