I'll upload a version with indent, sorry I didn't in the first place. I thought I had ran, but it didn't.

Created attachment 373647 [details] [review]
Patch of the implementation with indent

Created attachment 373648 [details] [review]
Patch of the implementation

Review of attachment 373646 [details] [review]:

Can you run gst-indent on these patch please.

I've ran indent with the following parameters:

        --braces-on-if-line \
	--case-brace-indentation0 \
	--case-indentation2 \
	--braces-after-struct-decl-line \
	--line-length80 \
	--no-tabs \
	--cuddle-else \
	--dont-line-up-parentheses \
	--continuation-indentation4 \
	--honour-newlines \
	--tab-size8 \
	--indent-level2 \
	--leave-preprocessor-space

Is that wrong?

Regards,

Created attachment 373649 [details] [review]
Patch of the implementation

Ran indent --braces-on-if-line --case-brace-indentation0 --case-indentation2 --braces-after-struct-decl-line --line-length80 --no-tabs --cuddle-else --dont-line-up-parentheses --continuation-indentation4 --honour-newlines --tab-size8 --indent-level2 --leave-preprocessor-space on all files (headers were left out last time).

Usually you simply run ./common/gst-indent . There is a commit hook also, not sure why it didn't work.

I've ran it now from common. It is the same as running the indent with the parameters above. Nothing has changed. Last patch should be indented correctly it seems.

Review of attachment 373649 [details] [review]:

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 ?

thanks for your review. I'll take a look asap.

BTW, I'm having a problem with timestamping which I think is a bug of dmssdemux and maybe you could help?

When I get a connection closed error, I ask the pipeline to restart, which makes the dmssssrc reconnect and restart sending packets to demux and upstream. However, since sometime has passed, the timestamp jumps forward a few seconds, which _seems_ to be the reason that everything freezes waiting on clock.

I wonder what should demux do in this case?

Regards,

Forward jumps, if this jump in timestamp if equal to the duration of the drop is fine. Though, if it's longer, you'll see freezes.

-- 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-bad/issues/787.