[Bug 770107] qtdemux: reports only one encryption system even if it can support more than one

[GStreamer (GNOME Bugzilla)]

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

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

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

--- Comment #20 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 352262
  --> https://bugzilla.gnome.org/attachment.cgi?id=352262
2/3 qtdemux: add context for a preferred protection

Again mostly style niggles, otherwise I'm fine with it:

> static void
>+gst_qtdemux_set_context (GstElement * element, GstContext * context)
>+{
>+  GstQTDemux *qtdemux = GST_QTDEMUX (element);
>+
>+  g_return_if_fail (GST_IS_CONTEXT (context));
>+
>+  if (g_strcmp0 (gst_context_get_context_type (context),
>+          "drm-preferred-decryption-system-id") == 0) {

Use gst_context_has_context_type(). If you rename 'context' to 'ctx' it might
even fit into a single line without getting indented ;)


>+    const GstStructure *s;
>+
>+    s = gst_context_get_structure (context);
>+    qtdemux->preferred_protection_system_id =
>+        g_strdup (gst_structure_get_string (s, "decryption-system-id"));

Should probably free any previously-saved id here?

What about locking?


>+  GST_TRACE_OBJECT (element, "chaining set_context to superclass %p or %p",
>+      GST_ELEMENT_GET_CLASS (element), parent_class);

Does this trace statement add anything? It looks like it should be removed.



>+static void
> qtdemux_parse_ftyp (GstQTDemux * qtdemux, const guint8 * buffer, gint length)
> {
>   /* counts as header data */
>@@ -3820,6 +3851,8 @@ qtdemux_parse_pssh (GstQTDemux * qtdemux, GNode * node)
>   event = gst_event_new_protection (sysid_string, pssh,
>       (parent_box_type == FOURCC_moov) ? "isobmff/moov" : "isobmff/moof");
>   for (i = 0; i < qtdemux->n_streams; ++i) {
>+    GST_TRACE_OBJECT (qtdemux,
>+        "adding protection event for stream %d and system %s", i, sysid_string);
>     g_queue_push_tail (&qtdemux->streams[i]->protection_scheme_event_queue,
>         gst_event_ref (event));
>   }
>@@ -5530,6 +5563,12 @@ gst_qtdemux_decorate_and_push_buffer (GstQTDemux * qtdemux,
>     GstEvent *event;
> 
>     while ((event = g_queue_pop_head (&stream->protection_scheme_event_queue))) {
>+#if (!GST_DISABLE_GST_DEBUG)
>+      const gchar *system_id = NULL;
>+      gst_event_parse_protection (event, &system_id, NULL, NULL);
>+      GST_TRACE_OBJECT (qtdemux, "pushing again protection event for system %s",
>+          system_id);
>+#endif

How about just:

        GST_TRACE_OBJECT (stream->pad, "pushing protection event: %"
GST_PTR_FORMAT, event);

or somesuch? Without the #if (which should probably also be #ifdef)


>+static gboolean
>+gst_qtdemux_run_query (GstElement * element, GstQuery * query,
>+    GstPadDirection direction)
>+{
>+  ...
>+  GValue res = { 0, };
>+
>+  g_value_init (&res, G_TYPE_BOOLEAN);
>+  ...
>+  return g_value_get_boolean (&res);
>+}

Strictly speaking we're missing a g_value_unset() here, but it's fine to skip
it in this case.


>+static void
>+gst_qtdemux_request_protection_context_if_needed (GstQTDemux * qtdemux,
>+    QtDemuxStream * stream)
>+{

We can probably drop the _if_needed() here?


>+  GST_TRACE_OBJECT (qtdemux, "currently we have detected %u protection systems",
>+      qtdemux->protection_system_ids->len);

Maybe drop "currently we have" (seems overly verbose).


>+  if (stream->protection_scheme_event_queue.length) {
>+    ...
>+    walk = stream->protection_scheme_event_queue.tail;
>+  } else {
>+    ...
>+    walk = qtdemux->protection_event_queue.tail;
>+  }
>+
>+  g_value_init (&event_list, GST_TYPE_LIST);
>+  for (; walk; walk = g_list_previous (walk)) {
>+    ...
>+  }

OOC, what is the reason we're walking the event queues backwards here?


>+  /*  2a) Query downstream with GST_QUERY_CONTEXT for the context and
>+   *      check if downstream already has a context of the specific type
>+   *  2b) Query upstream as above.
>+   */
>+  query = gst_query_new_context ("drm-preferred-decryption-system-id");
>+  st = (GstStructure *) gst_query_get_structure (query);

st = gst_query_writable_structure (query);



>+  gst_qtdemux_request_protection_context_if_needed (qtdemux, stream);
>+  if (qtdemux->preferred_protection_system_id != NULL) {
>+    guint i;

newline after declaration (gst-indent doesn't do that, sorry)

>+    for (i = 0; i < qtdemux->protection_system_ids->len; i++) {
>+      if (g_strcmp0 (g_ptr_array_index (qtdemux->protection_system_ids, i),
>+              qtdemux->preferred_protection_system_id) == 0) {
>+        const gchar *preferred_system_array[] =
>+            { qtdemux->preferred_protection_system_id, NULL };
>+        selected_system = gst_protection_select_system (preferred_system_array);
>+        break;
>+      }
>+    }
>+  }

Wonder if this could be written a bit nicer with some temp variables, but not a
dealbreaker :)

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