GNOME Bugzilla – Bug 797299
x264enc: Print full option-string applied to x264_encoder in debug log ...
Last modified: 2018-11-03 15:36:17 UTC
Created attachment 373952 [details] [review] x264enc: Print full option-string applied to x264_encoder in debug log ... ... and use x264 nal_unit_type enum instead of constant value. This debug log shows the following information : 0:00:00.227263644 12159 0xe722d0 INFO x264enc gstx264enc.c:1951:gst_x264_enc_set_profile_and_level:<x264enc0> Using x264_encoder info: 264 - core 144 r2533 c8a773e - H.264/MPEG-4 AVC codec - Copyleft 2003-2015 - http://www.videolan.org/x264.html - options: cabac=1 ref=2 deblock=1:0:0 analyse=0x3:0x113 me=hex subme=4 psy=1 psy_rd=1.00:0.00 mixed_ref=0 me_range=16 chroma_me=1 trellis=1 8x8dct=1 cqm=0 deadzone=21,11 fast_pskip=1 chroma_qp_offs et=0 threads=11 lookahead_threads=11 sliced_threads=1 slices=11 nr=0 decimate=1 interlaced=0 bluray_compat=0 constrained_intra=0 bframes=0 weightp=1 keyint=30 keyint_min=16 scenecut=0 intra_refresh=0 rc_lookahead=0 rc=cbr mbtree=0 bi trate=2048 ratetol=1.0 qcomp=0.60 qpmin=0 qpmax=69 qpstep=4 vbv_maxrate=2048 vbv_bufsize=1228 nal_hrd=none filler=0 ip_ratio=1.40 aq=1:1.00
Created attachment 373954 [details] [review] [1/2] x264enc: Don't use hard-coded constant value in nal type check Replace hard-coded constant values by enums in nal type check
Created attachment 373955 [details] [review] [2/2] x264enc: Print full option-string applied to x264_encoder in debug log This helps figure out precisely what options were applied to x264_encoder
Comment on attachment 373954 [details] [review] [1/2] x264enc: Don't use hard-coded constant value in nal type check Looks good to me but please explain in the commit message what you do exactly, why and what it fixes. Definitely makes sense to not hardcode this though, it will just fall apart sooner or later if x264enc behaviour changes slightly.
Comment on attachment 373955 [details] [review] [2/2] x264enc: Print full option-string applied to x264_encoder in debug log While the idea is good, just assuming that every SEI contains this as a NUL-terminated string at offset 25 is not great. Can we detect the SEI type and add some safety guards for this?
(In reply to Sebastian Dröge (slomo) from comment #3) > Comment on attachment 373954 [details] [review] [review] > [1/2] x264enc: Don't use hard-coded constant value in nal type check > > Looks good to me but please explain in the commit message what you do > exactly, why and what it fixes. Definitely makes sense to not hardcode this > though, it will just fall apart sooner or later if x264enc behaviour changes > slightly. Sorry, It is an obvious my mistake. I'll upload it again, including the commit message.
(In reply to Sebastian Dröge (slomo) from comment #4) > Comment on attachment 373955 [details] [review] [review] > [2/2] x264enc: Print full option-string applied to x264_encoder in debug log > > While the idea is good, just assuming that every SEI contains this as a > NUL-terminated string at offset 25 is not great. Can we detect the SEI type > and add some safety guards for this? I agree with this comment. I'll think about that and fix the patch. Thanks for the review :)
Created attachment 374019 [details] [review] [1/2] x264enc: Don't use hard-coded constant value in nal type check Added commit message.
Created attachment 374020 [details] [review] [2/2] x264enc: Print full option-string applied to x264_encoder in debug log Added parsing SEI nal unit and some safety check code with reference to h264parser
Review of attachment 374020 [details] [review]: ::: ext/x264/gstx264enc.c @@ +106,3 @@ #include <gst/video/gstvideometa.h> #include <gst/video/gstvideopool.h> +#include <gst/base/gstbytereader.h> You need to add $(GST_BASE_LIBS) and $(GST_BASE_CFLAGS) to Makefile.am for this, and gstbase_dep to meson.build @@ +1968,3 @@ + + /* skip uuid_iso_iec_11578 */ + if (!gst_byte_reader_skip (&br, 16)) { Could this be used to distinguish any SEI_USER_DATA_UNREGISTERED from the one we actually care for here? @@ +1975,3 @@ + sei_msg_payload = g_malloc (payload_size); + memcpy (sei_msg_payload, sei + gst_byte_reader_get_pos (&br), payload_size); + sei_msg_payload[payload_size - 1] = 0; Wouldn't you override the last character here in case the payload is not NUL-terminated already? @@ +2017,3 @@ +#ifndef GST_DISABLE_GST_DEBUG + guint8 *sei = NULL; + guint skip_bits = 0; This is bytes, not bits, isn't it?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-ugly/issues/21.