After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 615410 - x264 orders NALs differently than x264enc expects
x264 orders NALs differently than x264enc expects
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal blocker
: 0.10.15
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-04-10 23:29 UTC by Xavier Queralt
Modified: 2010-04-19 08:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Expect correct NAL ordering from x264 (3.02 KB, patch)
2010-04-10 23:31 UTC, Xavier Queralt
needs-work Details | Review
Adjust NAL ordering from libx264 (835 bytes, patch)
2010-04-13 12:29 UTC, Mark Nauwelaerts
none Details | Review
Adjust x264enc unit test NALU type checking (2.25 KB, patch)
2010-04-13 15:03 UTC, Mark Nauwelaerts
committed Details | Review
Adaptive x264 header NAL layout handling (3.81 KB, patch)
2010-04-16 19:13 UTC, Mark Nauwelaerts
committed Details | Review

Description Xavier Queralt 2010-04-10 23:29:54 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
Comment 1 Xavier Queralt 2010-04-10 23:31:34 UTC
Created attachment 158397 [details] [review]
Expect correct NAL ordering from x264
Comment 2 Sebastian Dröge (slomo) 2010-04-12 07:43:32 UTC
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?
Comment 3 Sebastian Dröge (slomo) 2010-04-12 07:48:35 UTC
Also this breaks the unit test it seems (not your patch but the x264 change)
Comment 4 Mark Nauwelaerts 2010-04-13 12:23:55 UTC
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
Comment 5 Mark Nauwelaerts 2010-04-13 12:29:00 UTC
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.
Comment 6 Mark Nauwelaerts 2010-04-13 15:03:45 UTC
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).
Comment 7 Sebastian Dröge (slomo) 2010-04-16 18:13:27 UTC
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?
Comment 8 Mark Nauwelaerts 2010-04-16 19:13:01 UTC
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.
Comment 9 Sebastian Dröge (slomo) 2010-04-17 08:27:22 UTC
Looks good to me, please commit both patches :)
Comment 10 Mark Nauwelaerts 2010-04-19 08:44:39 UTC
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.