[Bug 697806] avdec_h264 Fails during error recovery after packet loss leading to stream corruption

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=697806
  GStreamer | gst-libav | 1.0.5

--- Comment #27 from tcdgreenwood at hotmail.com 2013-12-06 00:26:51 UTC ---
Thanks for looking at my bug - I hope my patches were at least a little help.

I haven't been able to test it unfortunately as master currently isn't
compiling for me (I corrected a couple of problems and gave up - is this
expected? I might be able to get it to compile without -Werr) and the patch
fails on 1.2.  Is there a version that works on 1.2?  I'd like to test the
video quality and examine how PLC works with H.264 now - I have been surprised
how often we use PLC even with NACKs enabled.

>From a GStreamer point of view it seems to me that this patch means that you
are using allocations on a single GstVideoCodecFrame for multiple AVFrame
instances as you get it back with the context->reordered_opaque.  I'd like to
understand this better - it seems a clever solution.  Is this an
allowed/accepted pattern of usage?  I certainly didn't expect that this sort of
thing would be expected API usage as it seemed to make a certain presumption
about how the GstVideoCodecFrame worked.  I've generally tried to follow the
API docs and was concerned that it said "The buffer is owned by the frame and
references to the frame instead of the buffer should be kept." so I didn't
really want to mess with the buffer unless I had originally allocated myself.

In my patch I tried to correct what I saw as another potential issue which was
an a buffer allocated could continue to be used by libav even after you've
called the avcodec_decode_video2 (until the context has been free'd).  I don't
think this is broken right now (at least for H264), but I think it could be the
case in the future as far as I can tell.  I based this on what I saw of the API
docs and their code - especially the assumptions around the packet loss
concealment code.  What would happen if the buffer continued to be accessed? 
Is the buffer ref'd to stop it being released?  I may have gone too far in
allocating a unique AVFrame instance per frame as that is not the case with the
libav video decode example.

Have you thought how this will play with the new get_buffer2 API?  I think that
gstreamer still needs to upgrade libav to do that, but it seemed to me that
when that is done we wouldn't need to have references to pictures etc when we
get a release and could delete a bit of the code - just ref the buffer when
allocated into the libav buffer and release a ref on the associated buffer when
the free callback is called.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.