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

[Michael Niedermayer]

On Sun, Sep 26, 2010 at 05:51:20PM -0700, Jason Garrett-Glaser wrote:
> 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

>  h264.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 7f00856277a0376cf8598f00764c76bbf3e06da1  test.diff

ok with me if tested

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100927/5bd3762d/attachment.pgp>