Bug 192075 – [GStreamer][EME] CDMInstance should be shipped as a GstContext to the decryptors

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[GStreamer][EME] CDMInstance should be shipped as a GstContext to the decryptors

[Xabier Rodríguez Calvar]

Created attachment 355860 [details]
Patch

[Philippe Normand]

Comment on attachment 355860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355860&action=review

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:313
> +        GRefPtr<GstContext> context = adoptGRef(gst_element_get_context(GST_ELEMENT(self), "drm-cdm-instance"));
> +        if (context) {

If not context is found here, maybe you can try a context query and a need-context message, like described in the GstContext docs?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #2)
> Comment on attachment 355860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355860&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:313
> > +        GRefPtr<GstContext> context = adoptGRef(gst_element_get_context(GST_ELEMENT(self), "drm-cdm-instance"));
> > +        if (context) {
> 
> If not context is found here, maybe you can try a context query and a
> need-context message, like described in the GstContext docs?

Yes, I thought of that and it could make sense in other circumstances. In our case, as soon as we get the CDMInstance from WebCore, we set it so it should be available. If it isn't a query is not going to help cause nobody is going to answer it and a message would be useless as well cause the CDMInstance is something we don't ask but we get instead.

[Philippe Normand]

Comment on attachment 355860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355860&action=review

>>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:313
>>> +        if (context) {
>> 
>> If not context is found here, maybe you can try a context query and a need-context message, like described in the GstContext docs?
> 
> Yes, I thought of that and it could make sense in other circumstances. In our case, as soon as we get the CDMInstance from WebCore, we set it so it should be available. If it isn't a query is not going to help cause nobody is going to answer it and a message would be useless as well cause the CDMInstance is something we don't ask but we get instead.

A message sent by the decryptor could be handled in the player message handler.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 355860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355860&action=review
> 
> >>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:313
> >>> +        if (context) {
> >> 
> >> If not context is found here, maybe you can try a context query and a need-context message, like described in the GstContext docs?
> > 
> > Yes, I thought of that and it could make sense in other circumstances. In our case, as soon as we get the CDMInstance from WebCore, we set it so it should be available. If it isn't a query is not going to help cause nobody is going to answer it and a message would be useless as well cause the CDMInstance is something we don't ask but we get instead.
> 
> A message sent by the decryptor could be handled in the player message
> handler.

Obviously, but it is not going to be answered sooner, cause if we had it in the player, it would be set already in the pipeline.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #5)
> (In reply to Philippe Normand from comment #4)
> > Comment on attachment 355860 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355860&action=review
> > 
> > >>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:313
> > >>> +        if (context) {
> > >> 
> > >> If not context is found here, maybe you can try a context query and a need-context message, like described in the GstContext docs?
> > > 
> > > Yes, I thought of that and it could make sense in other circumstances. In our case, as soon as we get the CDMInstance from WebCore, we set it so it should be available. If it isn't a query is not going to help cause nobody is going to answer it and a message would be useless as well cause the CDMInstance is something we don't ask but we get instead.
> > 
> > A message sent by the decryptor could be handled in the player message
> > handler.
> 
> Obviously, but it is not going to be answered sooner, cause if we had it in
> the player, it would be set already in the pipeline.

It would be dead code.

[Charlie Turner]

Comment on attachment 355860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355860&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1332
> +            // FIXME: The decryptors should be able force attempt decryption after creation and being added to a pipeline as a way of trying to get they keys if needed.

This sentence needs rewording

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1218
> +    GST_LOG("CDM instance %p dispatched as context", m_cdmInstance.get());

This isn't a high frequency message, it would be better placed at the DEBUG level.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1226
> +#ifndef NDEBUG

Could remove the macro code and put an early return in for instance != m_cdmInstance, this would also make the logic more symmetrical with cdmInstanceAttached.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:912
> +void MediaPlayerPrivateGStreamerMSE::attemptToDecryptWithLocalInstance()

Since we're renaming things and don't have the distinction anymore of an argument instance vs. a class instance, this code would read nicer if the method was just called attemptToDecryptWithInstance, the local part doesn't communicate anything imo.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #7)
> Comment on attachment 355860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355860&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1332
> > +            // FIXME: The decryptors should be able force attempt decryption after creation and being added to a pipeline as a way of trying to get they keys if needed.
> 
> This sentence needs rewording

Yes.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1218
> > +    GST_LOG("CDM instance %p dispatched as context", m_cdmInstance.get());
> 
> This isn't a high frequency message, it would be better placed at the DEBUG
> level.

Yes, I should know my logging levels better ;)

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1226
> > +#ifndef NDEBUG
> 
> Could remove the macro code and put an early return in for instance !=
> m_cdmInstance, this would also make the logic more symmetrical with
> cdmInstanceAttached.

I'll try.

> > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:912
> > +void MediaPlayerPrivateGStreamerMSE::attemptToDecryptWithLocalInstance()
> 
> Since we're renaming things and don't have the distinction anymore of an
> argument instance vs. a class instance, this code would read nicer if the
> method was just called attemptToDecryptWithInstance, the local part doesn't
> communicate anything imo.

There's already an attemptToDecryptWithInstance, wdym?

[Xabier Rodríguez Calvar]

Created attachment 356019 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 356019 [details]
Patch

Uploaded by mistake.

[Xabier Rodríguez Calvar]

Created attachment 356023 [details]
Patch

[Thibault Saunier]

Why not simply a property on the decryptor element? As I see it, this is going through the GstContext API to simply set a property on a specific element. GstContext was designed to be able to share a common "context" on several elements in the pipeline.

To me, the "normal" way of doing that would be to have a "cdm-instance" property on the decryptor and in the MediaPlayerPrivate connect to `deep-element-added` and set the property from there.

What am I missing here? :-)

[Xabier Rodríguez Calvar]

(In reply to Thibault Saunier from comment #12)
> Why not simply a property on the decryptor element? As I see it, this is
> going through the GstContext API to simply set a property on a specific
> element. GstContext was designed to be able to share a common "context" on
> several elements in the pipeline.

For me this a common context shared by several elements in the pipeline, in this case the decryptors.

> To me, the "normal" way of doing that would be to have a "cdm-instance"
> property on the decryptor and in the MediaPlayerPrivate connect to
> `deep-element-added` and set the property from there.

Connecting to that signal would also imply inspecting the pipeline for the detach and unsetting the property when the instance is detached.

I don't have a strong opinion on GstContext vs property but in this case it looks better to me to use a context. Do you have a stronger opinion?

[Thibault Saunier]

(In reply to Xabier Rodríguez Calvar from comment #13)
> (In reply to Thibault Saunier from comment #12)
> > Why not simply a property on the decryptor element? As I see it, this is
> > going through the GstContext API to simply set a property on a specific
> > element. GstContext was designed to be able to share a common "context" on
> > several elements in the pipeline.
> 
> For me this a common context shared by several elements in the pipeline, in
> this case the decryptors.
> 
> > To me, the "normal" way of doing that would be to have a "cdm-instance"
> > property on the decryptor and in the MediaPlayerPrivate connect to
> > `deep-element-added` and set the property from there.
> 
> Connecting to that signal would also imply inspecting the pipeline for the
> detach and unsetting the property when the instance is detached.
> 
> I don't have a strong opinion on GstContext vs property but in this case it
> looks better to me to use a context. Do you have a stronger opinion?

No strong opinion no, just looked a bit odd at first sight, but I can understand your point stating that the CDMInstance is a shared context for all decryptor elements that is shared between instances.

[Philippe Normand]

Comment on attachment 356023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356023&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1209
> +    if (!m_pipeline) {
> +        GST_ERROR("no pipeline yet");
> +        ASSERT_NOT_REACHED();
> +        return;
>      }

Replace this with a RELEASE_ASSERT_WITH_MESSAGE() maybe?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1218
> +    GST_DEBUG("CDM instance %p dispatched as context", m_cdmInstance.get());

GST_DEBUG_OBJECT()

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1233
> +    GST_DEBUG("detaching CDM instance %p, setting empty context", m_cdmInstance.get());

GST_DEBUG_OBJECT

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #15)
> Comment on attachment 356023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356023&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1209
> > +    if (!m_pipeline) {
> > +        GST_ERROR("no pipeline yet");
> > +        ASSERT_NOT_REACHED();
> > +        return;
> >      }
> 
> Replace this with a RELEASE_ASSERT_WITH_MESSAGE() maybe?

I think in this case it is better to stick to the GST_ERROR and the ASSERT because should this happen in real life it would be better if playback just fails and we get that message than just a crash.
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1218
> > +    GST_DEBUG("CDM instance %p dispatched as context", m_cdmInstance.get());
> 
> GST_DEBUG_OBJECT()

Roger.
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1233
> > +    GST_DEBUG("detaching CDM instance %p, setting empty context", m_cdmInstance.get());
> 
> GST_DEBUG_OBJECT

Roger.

[Xabier Rodríguez Calvar]

Created attachment 356177 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 356177 [details]
Patch for landing

Clearing flags on attachment: 356177

Committed r238738: <https://trac.webkit.org/changeset/238738>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46373511>