I'm not sure how you envision "providing more than one structure at the caps for the outgoing stream" will work.

The way this works was a design decision and done on purpose.

We may have to add some way to query whether a key is available then or something when selecting the system.

I presume the version was supposed to be 1.8.0?

The idea was that qtdemux checks for which DRM schemes a decryptor exists, and exposes caps according to the highest-ranked one. This could probably be extended via some magic with GstContext to allow the application to give its own preferences per pipeline.

Why is it always exposing the first for you? You have decryptors for all 3, but only one is going to work?

(In reply to Tim-Philipp Müller from comment #1)
> I'm not sure how you envision "providing more than one structure at the caps
> for the outgoing stream" will work.

Mixed caps :)

(In reply to Sebastian Dröge (slomo) from comment #3)
> (In reply to Tim-Philipp Müller from comment #1)
> > I'm not sure how you envision "providing more than one structure at the caps
> > for the outgoing stream" will work.
> 
> Mixed caps :)

Is there any docs on mixed caps yet? I never looked at how it is supposed to work and handle intersections and similar operations we do in caps.


We could use a GstContext... or a bus sync message. Isn't a GstContext just like the order of the element ranks? Except that it would be per-pipeline rather than system wide. The message would be stream specific and seems more direct to the point IMHO.

(In reply to Tim-Philipp Müller from comment #1)
> I'm not sure how you envision "providing more than one structure at the caps
> for the outgoing stream" will work.
> 
> The way this works was a design decision and done on purpose.
> 
> We may have to add some way to query whether a key is available then or
> something when selecting the system.

Caps can have more than one structure, right? Usually people access only the first, meaning index 0.

My proposal would be to duplicate the structure for all the usable decryption systems. Pardon my ignorance if this is not possible or it does more harm than good.

> I presume the version was supposed to be 1.8.0?

I am trying with 1.8.2.

(In reply to Sebastian Dröge (slomo) from comment #2)
> The idea was that qtdemux checks for which DRM schemes a decryptor exists,
> and exposes caps according to the highest-ranked one. This could probably be
> extended via some magic with GstContext to allow the application to give its
> own preferences per pipeline.
> 
> Why is it always exposing the first for you?

According to this function that is used in qtdemux, it seems to use the first or am I wrong?:

const gchar *
gst_protection_select_system (const gchar ** system_identifiers)
{
  GList *decryptors, *walk;
  const gchar *retval = NULL;

  decryptors =
      gst_element_factory_list_get_elements (GST_ELEMENT_FACTORY_TYPE_DECRYPTOR,
      GST_RANK_MARGINAL);

  for (walk = decryptors; !retval && walk; walk = g_list_next (walk)) {
    GstElementFactory *fact = (GstElementFactory *) walk->data;
    retval = gst_protection_factory_check (fact, system_identifiers);
  }

  gst_plugin_feature_list_free (decryptors);

  return retval;
}

> You have decryptors for all 3,
> but only one is going to work?

We have for two, but it is a combination of JS app and WebKit that decides which one should be used (I didn't check yet how that can be handled).

Caps describing an actual output media format (sent downstream as CAPS event) must be fixed caps, which implies one structure.

(In reply to Tim-Philipp Müller from comment #6)
> Caps describing an actual output media format (sent downstream as CAPS
> event) must be fixed caps, which implies one structure.

I was afraid of this :)

So I guess a message answered thru the sync handler could be an option, or at least a list of possible alternative decryptors in another element of the structure that wouldn't break ABI.

Ok, my plans are:

* Creating and interface that will allow other demuxers to follow the same approach. This interface will have a method to set the preferred system id.
* qtdemux will implement this interface and at the moment of selecting the decryptor, it will send a message to the bus that shall be answered in the sync handler. If we don't get a satisfactory answer, we proceed as we do now, if not, we select the handler specified by the user. Of course, the interface will provide a helper to send that message.

How does this sound? If I don't get an answer I will begin with this.

Sounds like a job for GstContext to me, if you want to implement something like your plan. It has all the pieces you need for that already.


Alternatively this could be implemented with mixed-caps, but then someone would have to finish the design and implementation of those first.

Created attachment 335704 [details] [review]
1/1 gst-plugins-good

I am uploading this patch cause I had already cooked it but it is useless for the use case I explained. The patch checks for a preferred system id when it is going to configure the protected caps and if it is not available it will ask for the context. After that, if there is that preferred system, it will use it and if not it will use the other path.

Why do I say this is useless?

In our test case:

1. The stream is injected.
2. qtdemux detects the protection system ids, generates events for them and keeps them until the streams are set up.
3. It selects the system system (with or without my patch) and creates the protected caps.
4. The streams are set up and the events are emitted.

For us and according to the spec (the old one and I haven't checked the new one thoroughly but I bet it would be similar, you only have to see the diagram at https://www.w3.org/TR/encrypted-media/stack_overview.svg to realize that it is always the media system telling the upper layers if the media is encrypted or not) we cannot know in advance if the stream is encrypted and there is no [old] spec to see what happens when you find more than one encryption system (if you fire the needkey events and so on).

Summing up, I will close this bug for now and leave the patch there in case you find it is interesting now or in the future.

Created attachment 341420 [details] [review]
1/1 gst-plugins-good qtdemux patch

I got this patch working with WebKit for ARM and thanks to it I manage to pass some more YouTube tests.

The good thing is that this patch does not modify any API/ABI so it won't make other implementations stop working.

Created attachment 341576 [details] [review]
2/2 gstreamer

I had forgotten to add this patch that would be also needed for GStreamer core.

Created attachment 344417 [details] [review]
2/2 gstreamer

(In reply to Xabier Rodríguez Calvar from comment #14)
> Created attachment 344417 [details] [review] [review]
> 2/2 gstreamer

I was about to put these patches into WebKit internal jhbuild and realized that the patch was duplicated and incorrectly labeled. Now both are ok.

Created attachment 352261 [details] [review]
1/3 protection: added function to filter system ids

Previous patch for gstreamer by Calvaris rebased to latest master (e3ab5e4d)

Created attachment 352262 [details] [review]
2/3 qtdemux: add context for a preferred protection

Previous patch for gstreamer by Calvaris rebased to latest gst-plugins-good master (4f713717), plus some minor modifications by Enrique to allow selection of undetected key systems.

Created attachment 352263 [details] [review]
3/3 qtdemux: also push buffers without encryption info instead of dropping them

Test "17. PlayReadyH264Video" in YouTube leanback EME conformance tests 2016
for H.264[1] uses a media file[2] with cenc encryption whose first two 'moof'
boxes have no encryption information (no 'saiz' and 'saio' boxes).

Those boxes are actually not encrypted and the current qtdemux implementation
was just dropping them, breaking the test use case.

This patch detects those kind of situations and just lets the unencrypted
buffers pass. Of course, this needs some collaboration by the decryptors,
which should also do the same and not to try to decrypt those clear buffers.

[1] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?test_type=encryptedmedia-test&webm=false
[2] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/media/oops_cenc-20121114-142.mp4

This patch depends on the previous two patches being applied.

Comment on attachment 352261 [details] [review]
1/3 protection: added function to filter system ids

Looks fine, just some minor niggles:

> 
>+gchar **
>+gst_protection_filter_systems_by_available_decryptors (const gchar **
>+    system_identifiers)
>+{

Need a gtk-doc blurb here for the new API with 'Since: 1.14' marker at end, and add it to the -sections.txt file in docs/gst/ 

>+  GList *decryptors, *walk;
>+  gchar **retval;
>+  guint i = 0, decryptors_number;
>+
>+  decryptors =
>+      gst_element_factory_list_get_elements (GST_ELEMENT_FACTORY_TYPE_DECRYPTOR,
>+      GST_RANK_MARGINAL);
>+
>+  decryptors_number = g_list_length (decryptors);
>+  retval = g_new (gchar *, decryptors_number + 1);
>+
>+  GST_TRACE ("found %u decrytors", decryptors_number);
>+
>+  for (walk = decryptors; walk; walk = g_list_next (walk)) {
>+    GstElementFactory *fact = (GstElementFactory *) walk->data;
>+
>+    const char *found_sys_id =
>+        gst_protection_factory_check (fact, system_identifiers);
>+    GST_TRACE ("factory %s is valid for %s", GST_OBJECT_NAME (fact),
>+        found_sys_id);

Newline after variable declaration above please, and you can probably use GST_DEBUG or GST_LOG here, unless this is called a lot.


>--- a/gst/gstprotection.h
>+++ b/gst/gstprotection.h
>@@ -71,5 +71,8 @@ GstProtectionMeta * gst_buffer_add_protection_meta (GstBuffer * buffer,
> GST_EXPORT
> const gchar * gst_protection_select_system (const gchar ** system_identifiers);
> 
>+gchar ** gst_protection_filter_systems_by_available_decryptors (
>+    const gchar ** system_identifiers);
>+

This needs a GST_EXPORT, and you'll also need to run 'make update-exports' (there should be a new entry in the win32 .def file for the new function).

Comment on attachment 352262 [details] [review]
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 :)

Created attachment 352272 [details] [review]
1/3 protection: added function to filter system ids (review suggestions applied)

Thanks for the review. Here's the 1/3 patch with the applied suggestions.

(In reply to Tim-Philipp Müller from comment #20)
> >+  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?

Cause that way, we queue the protection events in the same order they are found in the media. If we don't walk the queue backwards, we'll have to do it in the client, but I think pushing the events in the same order they are found is a good idea.

Created attachment 352274 [details] [review]
1/3 protection: added function to filter system ids (review suggestions applied)

Created attachment 352311 [details] [review]
2/3 qtdemux: add context for a preferred protection (review suggestions applied)

Applied the review suggestions except the event queues backward walking (Calvaris commented the rationale of iterating backwards) and the lack of locking.

We discussed in person about this lack of locking and realized that gst_qtdemux_set_context() could actually be called from any thread (eg: by the application from the main thread, not just by qtdemux in the streaming thread) and that's why some locking on the object (on qtdemux) would be needed. However, locks aren't recursive and it's not clear how we would detect if recursion is happening and the lock is already acquired. The summary of the discussion was to forget about locking by now and just assume that the only caller of gst_qtdemux_set_context() will be the streaming thread.

Created attachment 354242 [details] [review]
2/3 qtdemux: add context for a preferred protection (review suggestions applied)

I reworked the patch to make it more efficient since there were some checks that had sense before but they don't since we are going to try what is preferred by the context even it wasn't reported as encryption system by the media. I also fixed the case about not having a recommendation from the app that was ending in an error. As it is now:

1- runs the context query if no context was set to the demuxer with all the encryption information found on the media
2- checks if there's a decryptor for the preferred encryption system if any. If it can find the decryptor, ok, if it can't then it warns
3- if there is no preferred selected system, it runs the old algorithm to keep the same behavior that it had so far

According to our last conversations with Tim during the hackfest, this would "landable" as it is now.

Comment on attachment 352274 [details] [review]
1/3 protection: added function to filter system ids (review suggestions applied)

>+/**
>+ * gst_protection_filter_systems_by_available_decryptors

This needs a colon at the end I believe?

>+ * Returns: (transfer full): A null terminated array containing all the
>+ * @system_identifiers supported by the set of available decryptors.

Apologies if I've asked this before already, but it seems if there are not matches we currently return an array with str[0] = NULL. Wouldn't it be better to return NULL in case there are no matches?

Created attachment 354610 [details] [review]
1/3 protection: added function to filter system ids (review suggestions applied)

Complied with suggestions

Comment on attachment 354610 [details] [review]
1/3 protection: added function to filter system ids (review suggestions applied)

commit bda8440f1f0de5be8036acfd78ec1d72929558ec
Author: Xabier Rodriguez Calvar <calvaris@igalia.com>
Date:   Wed Jun 28 09:54:56 2017 +0200

    protection: add function to filter system ids
    
    gst_protection_filter_systems_by_available_decryptors() takes an array
    of strings and returns a new array of strings filtered by the available
    decryptors for them so the ones you get are the ones that you should be
    able to decrypt.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=770107

Comment on attachment 354242 [details] [review]
2/3 qtdemux: add context for a preferred protection (review suggestions applied)

There's a funny thing with W3C tests that renders 2/3 patch useless without changes, therefore I am marking it as obsolete until I change it for the purpose I'll state next.

We use W3C tests in WebKit upstream and media used in them is encrypted with Widevine and PlayReady but tests will decrypt them with ClearKey. We don't provide decryptors neither for PlayReady nor for Widevine (yet) upstream so in this case the patch does not trigger the context query because it thinks there is no decryptor for that. I'll rework this to trigger the query anyway which should be harmless and won't change current behavior.

Created attachment 354878 [details] [review]
2/3 qtdemux: add context for a preferred protection (review suggestions applied)

Removed the requirement of having a valid decryptor to run the context query. In this case we could be able do decrypt with another available decryptor that is not reported by the media.

Review of attachment 354878 [details] [review]:

Looks good, I just have the impression locking will need fixing in all the protection system related code as thread safety looks fishy there, I do not see any locking around the arrays/fields which doesn't sound right at first sight. This patch doesn't make it better nor worth afaics so I would not block on that.

Review of attachment 354878 [details] [review]:

Looks good, I just have the impression locking will need fixing in all the protection system related code as thread safety looks fishy there, I do not see any locking around the arrays/fields which doesn't sound right at first sight. This patch doesn't make it better nor worth afaics so I would not block on that.

Review of attachment 352263 [details] [review]:

This makes sense, let's get it in.

All patches have been committed... does this need to still be open?

Sorry about that, something went wrong while merging with git-bz I have the impression. Let's close and add commit refs:


commit ee4b45da24cb7465b416c230597f8efc7b2c45cb
Author: Xabier Rodriguez Calvar <calvaris@igalia.com>
Date:   Wed Jun 21 17:59:21 2017 +0200

    qtdemux: add context for a preferred protection

    qtdemux selected the first system corresponding to a working GStreamer
    decryptor. With this change, before selecting that decryptor, qtdemux
    will check if it has context (a preferred decryptor id) and if not, it
    will request it.

    The request includes track-id, available key system ids for the
    available decryptors and even the events so that the init data is
    accessible.

    [eocanha@igalia.com: select the preferred protection system even if not available]

    Test "4. ClearKeyVideo" in YouTube leanback EME conformance tests 2016 for
    H.264[1] uses a media file[2] with cenc encryption which embeds 'pssh' boxes
    with the init data for the Playready and Widevine encryption systems, but not
    for the ClearKey encryption system (as defined by the EMEv0.1b spec[3] and with
    the encryption system id defined in [4]).

    Instead, the ClearKey encryption system is manually selected by the web page
    code (even if not originally detected by qtdemux) and the proper decryption key
    is dispatched to the decryptor, which can then decrypt the video successfully.

    [1] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?test_type=encryptedmedia-test&webm=false
    [2] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/media/car_cenc-20120827-86.mp4
    [3] https://dvcs.w3.org/hg/html-media/raw-file/eme-v0.1b/encrypted-media/encrypted-media.html#simple-decryption-clear-key
    [4] https://www.w3.org/Bugs/Public/show_bug.cgi?id=24027#c2

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

---

commit 844423ff99e281fc831303b92861ed43ce5c1518
Author: Enrique Ocaña González <eocanha@igalia.com>
Date:   Sat May 20 16:55:40 2017 +0000

    qtdemux: also push buffers without encryption info instead of dropping them

    Test "17. PlayReadyH264Video" in YouTube leanback EME conformance tests 2016
    for H.264[1] uses a media file[2] with cenc encryption whose first two 'moof'
    boxes have no encryption information (no 'saiz' and 'saio' boxes).

    Those boxes are actually not encrypted and the current qtdemux implementation
    was just dropping them, breaking the test use case.

    This patch detects those kind of situations and just lets the unencrypted
    buffers pass. Of course, this needs some collaboration by the decryptors,
    which should also do the same and not to try to decrypt those clear buffers.

    [1] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?test_type=encryptedmedia-test&webm=false
    [2] http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/media/oops_cenc-20121114-142.mp4

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

(In reply to Thibault Saunier from comment #35)
> Let's close

Let me help you with that? :P