[Bug 797140] Add dmss protocol plugins
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Thu Sep 13 23:57:27 UTC 2018
https://bugzilla.gnome.org/show_bug.cgi?id=797140
Nicolas Dufresne (ndufresne) <nicolas at ndufresne.ca> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #373649|none |needs-work
status| |
--- Comment #9 from Nicolas Dufresne (ndufresne) <nicolas at ndufresne.ca> ---
Review of attachment 373649:
--> (https://bugzilla.gnome.org/review?bug=797140&attachment=373649)
In the commit message, it would be nice to give as much context as possible,
e.g. basic of the protocol, links if any is available. If we would need to spec
or reverse engineering note to fix it. I have made couple of initial comments,
these are recurrent in your code, so take try to apply to the entire patch.
What I would really to see in this patch is an effort to put as much
information as possible about the protocol being implemented, so it can be
maintained. Also, try and craft a protocol header that would hold all the magic
number and tables that are spread out all over the code.
::: gst/dmss/gstdmssdemux.c
@@ +35,3 @@
+ GST_STATIC_CAPS (\
+ "video/x-h264, stream-format=(string)byte-stream," \
+ " alignment=(string)nal;" \
We are about to tighten the definition of alignment=nal as being 1 nal per
GstBuffer, is that the case ?
@@ +161,3 @@
+
+ timestamp = epoch;
+ timestamp *= 1000ull * 1000ull * 1000ull;
ull is ideal for 32/64 bit. GLib provides bunch of _CONSTANT() marco to handle
this and void compiler warnings. In this, it seems like you have exactly 1s, in
which case GST_SECOND is much more readable.
@@ +162,3 @@
+ timestamp = epoch;
+ timestamp *= 1000ull * 1000ull * 1000ull;
+ timestamp += (((guint64) ts) % 1000) * 1000ull * 1000ull;
Use problem second macro, GST_SECOND/GST_MSECOND, etc, it's also obscure enough
that it's worth a comment. Is this to avoid negative timestamp ?
@@ +174,3 @@
+
+static guint64
+gst_dmss_demux_find_extended_header_value (guint8 prefix,
Add a comment before this, that explains what you are parsing. It would nice
nicer to avoid magic numbers, and use protocol defines.
@@ +355,3 @@
+ while (size >= prologue_size + minimum_dhav_size) {
+ prologue =
+ gst_adapter_map (demux->adapter, prologue_size + minimum_dhav_size);
This can fail, you need to check the return value.
@@ +378,3 @@
+
+ dhav_packet_size =
+ GUINT32_FROM_LE (*(guint32 *) & prologue[prologue_size + 12]);
I'm worried you might be doing unaligned access here. Use
GST_READ_UINT32_BE(addr) instead.
@@ +392,3 @@
+ }
+
+ GST_DEBUG
Use GST_DEBUG_OBJECT (demux, ...
Applies for all the patches, not just this line. We have the same for warnings.
This helps a lot when you have multiple instances.
@@ +395,3 @@
+ ("DHAV packet (checking if downloaded) type: %.02x DHAV size: %d head
size: %d body size: %d",
+ (int) dhav_packet_type, (int) dhav_packet_size, (int) dhav_head_size,
+ (int) dhav_body_size);
Instead of downcasting, use the formatters, "string %" G_GSIZE_FORMAT " more
string" ...
@@ +518,3 @@
+ /* GST_BUFFER_DURATION (buffer) = 30; */
+
+ GST_LOG_OBJECT (demux, "%s buffer of size %" G_GSIZE_FORMAT ", ts %"
GST_TIME_FORMAT /*", dur %" GST_TIME_FORMAT */
Missing some cleanup ?
@@ +531,3 @@
+ /* if (!demux->audiosrcpad) */
+ /* { */
+ /* } */
Forgot to cleanup ?
@@ +605,3 @@
+{
+ /* GstDmssDemux *demux = GST_DMSS_DEMUX (element); */
+ /* GstStateChangeReturn ret; */
Remove the virtual implementation if you don't need it,
@@ +613,3 @@
+gst_dmss_demux_sink_query (GstPad * pad, GstObject * parent, GstQuery * query)
+{
+ GST_DEBUG ("%s:%d %s %d", __FILE__, __LINE__, __func__,
GST_DEBUG macros already do that, no need to repeat.
@@ +614,3 @@
+{
+ GST_DEBUG ("%s:%d %s %d", __FILE__, __LINE__, __func__,
+ (int) GST_QUERY_TYPE (query));
No downcasting, and use g_type_name() for debug traces.
@@ +647,3 @@
+ /* gst_util_uint64_scale (GST_SECOND,
demux->output_buffer_duration_n, */
+ /* demux->output_buffer_duration_d); */
+ latency = 2000 * 1000ul * 1000ul;
That 2s right ? 2 * GST_SECOND. Missing some cleanup also, any reason to
hardcode ?
@@ +678,3 @@
+gst_dmss_demux_send_event (GstElement * element, GstEvent * event)
+{
+ GST_DEBUG ("%s:%d %s", __FILE__, __LINE__, __func__);
Again, also, remove that function, it's not needed clearly.
@@ +711,3 @@
+ /* gst_segment_init (&dvdemux->time_segment, GST_FORMAT_TIME); */
+ /* dvdemux->discont = TRUE; */
+ /* res = gst_dvdemux_push_event (dvdemux, event); */
Missing cleanup ?
@@ +1034,3 @@
+ offset = 0;
+ while (offset != body_size) {
+ if ((size = g_socket_receive (socket, &buffer[offset % sizeof (buffer)],
It's quite weird a demuxer that runs a socket, I guess if you had some
documentation we could understand.
@@ +1040,3 @@
+ sizeof (buffer)) ? sizeof (buffer) -
+ (offset % sizeof (buffer)) : (body_size -
+ offset), cancellable, err)) < 0)
Indentation is screwed up, let's not blindly trust gst-indent ;-D
::: gst/dmss/gstdmsssrc.c
@@ +212,3 @@
+ src = GST_DMSS_SRC (bsrc);
+
+ caps = (filter ? gst_caps_ref (filter) : gst_caps_new_any ());
This is not correct, and also not needed. Drop this virtual BaseSrc will do the
right thing (you have fixed caps in your template).
@@ +228,3 @@
+ case PROP_HOST:
+ if (!g_value_get_string (value)) {
+ g_warning ("host property cannot be NULL");
Use g_return_if_fail() maybe ?
@@ +324,3 @@
+ if (!GST_CLOCK_TIME_IS_VALID (src->last_ack_time) ||
+ current_time - src->last_ack_time > GST_SECOND) {
+ // send nop
No C++ comment please. Also, did you mean no-op.
@@ +629,3 @@
+ 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0,
+ 0x04, 0x02, 0x03, 0x00, 0x01, 0xa1, 0xaa
Comment what you understand, I guess this is has been reversed ?
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
More information about the gstreamer-bugs
mailing list