[Bug 795424] qtdemux: Add MSE-style flush

[GStreamer (GNOME Bugzilla)]

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

--- Comment #17 from Thibault Saunier <tsaunier at gnome.org> ---
Review of attachment 371186:
 --> (https://bugzilla.gnome.org/review?bug=795424&attachment=371186)

I stopped reviewing at that point, will come back to it after we discuss
possible ways to simplify it as a whole.

::: tests/check/elements/qtdemux.c
@@ +30,3 @@
 #include <fcntl.h>
+#include <gst/app/app.h>
+#include <pthread.h>

Not required if you use GLib types

@@ +1297,3 @@
+  GstMemory *mem;
+  GstBuffer *buf;
+  mem = read_file_to_memory ("ibpibp-dash.mp4");

This seems to be leaked. Also that function doesn't exist anywhere yet, I know
it comes from some other commit.

@@ +1305,3 @@
+typedef struct _Barrier
+{
+  pthread_cond_t cond;

You should use GLib mutexes and conds here.

@@ +1316,3 @@
+  int ret;
+  ret = pthread_mutex_init (&barrier->mutex, NULL);
+  g_assert (ret == 0);

Use libcheck assertion functions (fail_unless and friends) instead of GLib
ones.

Reporting only here, but there are many occurrences of that.

@@ +1340,3 @@
+    pthread_cond_wait (&barrier->cond, &barrier->mutex);
+  }
+  if (barrier->errored) {

I would rather "assert" in the hanlder, and do nothin in that code path, maybe
you could even remove that field all together then.

@@ +1346,3 @@
+  }
+
+  /* Reset the barrier so that it can be reused. */

Then reset `->error` too?

@@ +1371,3 @@
+static GstPadProbeReturn
+lift_barrier_on_empty_buffer (GstPad * pad, GstPadProbeInfo * info,
+    void *userdata)

I do not get where the empty buffer comes from.

@@ +1380,3 @@
+
+  buffer = info->data;
+  if (gst_buffer_n_memory (buffer) == 0) {

Would be more obvious to check `gst_buffer_get_size()` here I think.

@@ +1475,3 @@
+  g_printerr ("ERROR from element %s: %s\n",
+      GST_OBJECT_NAME (msg->src), err->message);
+  g_printerr ("Debugging info: %s\n", (dbg_info) ? dbg_info : "none");

You could used `g_error` instead.

@@ +1489,3 @@
+
+  appsrc_pad = gst_element_get_static_pad (GST_ELEMENT (tester->appsrc),
"src");
+  g_assert_nonnull (appsrc_pad);

Unnecessary check.

@@ +1508,3 @@
+
+static void
+qtdemux_fragment_tester_mse_flush (QtDemuxFragmentTester * tester)

Those are just flush, please update using normal flushes in that function.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.