[Bug 797299] New: x264enc: Print full option-string applied to x264_encoder in debug log ...

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Bug ID: 797299
Summary: x264enc: Print full option-string applied to
x264_encoder in debug log ...
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-ugly
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

Created attachment 373952
--> https://bugzilla.gnome.org/attachment.cgi?id=373952&action=edit
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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@zen.co.uk
Attachment #373952|0 |1
is patch| |
Attachment #373952|application/octet-stream |text/plain
mime type| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Yeongjin Jeong <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373952|0 |1
is obsolete| |

--- Comment #1 from Yeongjin Jeong <***@gmail.com> ---
Created attachment 373954
--> https://bugzilla.gnome.org/attachment.cgi?id=373954&action=edit
[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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

--- Comment #2 from Yeongjin Jeong <***@gmail.com> ---
Created attachment 373955
--> https://bugzilla.gnome.org/attachment.cgi?id=373955&action=edit
[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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net
Attachment #373954|none |needs-work
status| |

--- Comment #3 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 373954
--> https://bugzilla.gnome.org/attachment.cgi?id=373954
[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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373955|none |needs-work
status| |

--- Comment #4 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 373955
--> https://bugzilla.gnome.org/attachment.cgi?id=373955
[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?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Severity|normal |enhancement

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

--- Comment #5 from Yeongjin Jeong <***@gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #3)

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.

Sorry, It is an obvious my mistake. I'll upload it again, including the commit
message.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

--- Comment #6 from Yeongjin Jeong <***@gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #4)

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?

I agree with this comment. I'll think about that and fix the patch.

Thanks for the review :)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Yeongjin Jeong <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373954|needs-work |none
status| |
Attachment #373954|0 |1
is obsolete| |

--- Comment #7 from Yeongjin Jeong <***@gmail.com> ---
Created attachment 374019
--> https://bugzilla.gnome.org/attachment.cgi?id=374019&action=edit
[1/2] x264enc: Don't use hard-coded constant value in nal type check

Added commit message.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Yeongjin Jeong <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373955|needs-work |none
status| |
Attachment #373955|0 |1
is obsolete| |

--- Comment #8 from Yeongjin Jeong <***@gmail.com> ---
Created attachment 374020
--> https://bugzilla.gnome.org/attachment.cgi?id=374020&action=edit
[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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #374019|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #374020|none |needs-work
status| |

--- Comment #9 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 374020:
--> (https://bugzilla.gnome.org/review?bug=797299&attachment=374020)

::: 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?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797299

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #10 from GStreamer system administrator <***@gstreamer.net> ---
-- 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.