GNOME Bugzilla – Bug 615410
x264 orders NALs differently than x264enc expects
Last modified: 2010-04-19 08:45:15 UTC
In [1] the order the NALs are returned by x264_encoder_headers is changed from SEI, SPS and PPS to SPS, PPS and SEI. Thus, the header is incorrect and cannot be handled. This appears already in the version of libx264 in debian unstable (0.92). The patch attached corrects the expected order. Maybe it should also handle the old ordering by checking x264's version at configure time, what do you think? [1] http://git.videolan.org/?p=x264.git;a=commit;f=encoder/encoder.c;h=c6de86497cdd7b7f3cce7d8a95d723c7d0c9f505
Created attachment 158397 [details] [review] Expect correct NAL ordering from x264
The patch itself is fine but it will break x264enc with older versions of x264. Is there some runtime version check for x264 maybe? Otherwise, could you change your patch to allow both orderings?
Also this breaks the unit test it seems (not your patch but the x264 change)
In any case, it makes sense to reduce some hardcoding (i.e. 0, 1, 2): commit 5fd1796fda4edc7e1536feb7a71fad4e6b1dcf12 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Apr 13 13:16:41 2010 +0200 x264enc: parameterize libx264 provided NAL layout in codec-data creation
Created attachment 158594 [details] [review] Adjust NAL ordering from libx264 AFAICS, there is no runtime way to detect version, so attached patch uses compile time version to determine NAL layout.
Created attachment 158611 [details] [review] Adjust x264enc unit test NALU type checking This should make the unit test less sensitive to NALU type ordering (at least w.r.t. SEI) and as such no longer break with a recent libx264 (as mentioned above).
Review of attachment 158594 [details] [review]: ::: ext/x264/gstx264enc.c @@ +785,1 @@ #define SEI_NI 0 This will fail if x264enc is built against an older x264 and then is used with a newer version. Is there some runtime check for this?
Created attachment 158918 [details] [review] Adaptive x264 header NAL layout handling So this version uses some poor man's trial-and-error to aim for the proper NAL order.
Looks good to me, please commit both patches :)
commit 6ebb4e0fedf0ee8c75eba9269600f61caf6b83d7 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Apr 13 16:59:03 2010 +0200 x264enc: adaptive NALU type checking In particular, be less picky about SEI NALU order, which makes test more robust with respect to changes in libx264. See also #615410. commit d17388afa99932e29959b87df16c1421f6e767f3 Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Fri Apr 16 21:10:58 2010 +0200 x264enc: adaptive x264 header NAL layout handling Fixes #615410.