GNOME Bugzilla – Bug 625557
x264enc doesn't flush delayed frames properly
Last modified: 2010-08-02 08:01:59 UTC
It seems when I updated the defaults in #607798 a bug in flushing the frames whose encoding is delayed due to b-frames or so was uncovered by make check that was missed. When using certain features of x264enc, libx264 requires a frame or so of delay internally before pushing out an encoded frame such that it can use said features. When we hit EOS or whatever and want to have these frames output from the encoder, we're supposed to call x264_encoder_encode() with a NULL input picture until x264_encoder_delayed_frames() returns 0, indicating that all delayed frames have been encoded and pushed out. See the following patch which should fix it and does for me. I wasn't sure about the i_nal > 0 test in the expression, but I left it in case removing it would introduce a regression. If it's OK to remove it, go ahead.
Created attachment 166758 [details] [review] Fix frame flushing bug
Created attachment 166810 [details] [review] Fix frame flushing bug It seems that the i_nal > 0 check didn't work possibly because we can have > 1 frame of delay in the x264 encoder. This meant that one real frame would be pushed in and no NALs would come out, one NULL frame would be pushed in and no NALs would come out and we'd consider it flushed but with no output buffers. Using x264_encoder_delayed_frames() would then cause a third call to x264_encoder_encode() with a NULL input frame which would finally return the encoded frame (i.e. > 0 NALs). That's my theory anyway. As such, we don't need the i_nal check but it's only supported in libx264 APIs versions 71 and higher (around 2009-08) so I've added in a bit of ifdeffery to at least be able to flush one frame though that method is broken. Perhaps we can figure out some more reliable way of flushing when the API is not available. I think the delay can be calculated from x264 parameters.
So if after pushing in one frame, it is necessary to push NULL more than once, it means that x264_encoder_delayed_frames() > 1, so "the number of currently delayed (buffered) frames" (as documented in the header files) is then > number of total frames provided (= 1), which is some buffering ... Suffice it to say some weirdery in API might be going on, but if it works, use it, so attached patch is fine.
Pushed. (Sorry, I forgot to put the bug number in the commit message.) Module: gst-plugins-ugly Branch: master Commit: a441e5b6ef5575a178c27f2c993a893e572a6191 URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-ugly/commit/?id=a441e5b6ef5575a178c27f2c993a893e572a6191 Author: Robert Swain <robert.swain@collabora.co.uk> Date: Thu Jul 29 09:41:49 2010 +0200 x264enc: Fix flushing of delayed frames x264_encoder_encode() should be called with a NULL picture until at least x264_encoder_delayed_frames() returns 0. This fixes what appeared to be a regression in make check due to the recent change in defaults which enabled b-frames and b-pyramid, both of which I believe increase the number of delayed frames when encoding.