[GStreamer][EME] CDMInstance should be shipped as a GstContext to the decryptors
Created attachment 355860 [details] Patch
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?
(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.
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.
(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.
(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.
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.
(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?
Created attachment 356019 [details] Patch
Comment on attachment 356019 [details] Patch Uploaded by mistake.
Created attachment 356023 [details] Patch
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? :-)
(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?
(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.
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
(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.
Created attachment 356177 [details] Patch for landing
Comment on attachment 356177 [details] Patch for landing Clearing flags on attachment: 356177 Committed r238738: <https://trac.webkit.org/changeset/238738>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46373511>