Bug 162908 – [GStreamer][EME] Add EME support to base private player

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

Add encryption key management support.

[Enrique Ocaña]

Created attachment 290605 [details]
Patch

[Enrique Ocaña]

Comment on attachment 290605 [details]
Patch

Wait until all the patches in 157314 are ready.

[Xabier Rodríguez Calvar]

Comment on attachment 290605 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:111
> +#if ENABLE(ENCRYPTED_MEDIA_V2)

You can move this to the top of the function. If this is the only thing to do we can even compile the check out of no EME is activated.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:114
> +        gst_element_register(0, "webkitclearkey", GST_RANK_PRIMARY + 100, WEBKIT_TYPE_MEDIA_CK_DECRYPT);

0 -> nullptr

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283
> +            gboolean valid = gst_structure_get(structure, "data", GST_TYPE_BUFFER, &data,
> +                "key-system-id", G_TYPE_STRING, &keySystemId, nullptr);

According to the gst_structure_get: "For strings and boxed types you will receive a copy which you will need to release with either g_free() or the suitable function for the boxed type." so it seems we are leaking things here. GRefPtr with outPtr seems the solution.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:290
> +#if ENABLE(ENCRYPTED_MEDIA_V2)

This is useless.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1095
> +        GST_DEBUG("no event handler for key needed");

GST_INFO?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1114
> +    if (!supportsKeySystem(keySystem, emptyString()))
> +        return nullptr;
> +
> +    GST_DEBUG("creating key session for %s", keySystem.utf8().data());
> +    return nullptr;

WAT? This log doesn't make sense if you return null. Actually, nothing on the function makes sense. Even it is the default implementation we should just return nullptr and maybe, only maybe, add a GST_TRACE warning that we are returning null.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1126
> +    GST_DEBUG("Checking for KeySystem support with %s and type %s", keySystem.utf8().data(), mimeType.utf8().data());

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:224
> +    CDMSession* m_cdmSession;

Who has ownership of this? Does it make sense to add a smart pointer here? It does not seem to be freed anywhere.

[Xabier Rodríguez Calvar]

Comment on attachment 290605 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:285
> +            if (!valid || !gst_buffer_map(data, &mapInfo, GST_MAP_READ))

UNLIKELY?

[Enrique Ocaña]

Comment on attachment 290605 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:224
>> +    CDMSession* m_cdmSession;
> 
> Who has ownership of this? Does it make sense to add a smart pointer here? It does not seem to be freed anywhere.

MediaKeySession holds a unique_ptr<CDMSession>. The rest of the classes with access to the CDMSession must live with just a raw pointer to it.

[Enrique Ocaña]

Created attachment 291767 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 291767 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1148
> +    if (!supportsKeySystem(keySystem, emptyString()))
> +        GST_DEBUG("KeySystem not supported, returning null CDMSession");
> +    else
> +        GST_TRACE("KeySystem supported, but returning null CDMSession anyway");

It doesn't make sense that this function is here and return null regardless the result of supportsKeySystem. This is wrong. I guess this is supposed to NOT happen because I guess subclasses should override it.

If it's compulsory to override it and we are fishing or compile errors, ASSERT_NOT_REACHED and return nullptr.

If it is not compulsory to override it and this is still correctly called from somewhere else, then just return nullptr, GST_DEBUG to make clear that we are at the base class returning always null and do not ever call supportsKeySystem.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1161
> +    GST_DEBUG("Checking for KeySystem support with %s and type %s", keySystem.utf8().data(), mimeType.utf8().data());

Something similar applies here.

If this is supposed to be compulsory overridden, then ASSERT_NOT_REACHED and return false

If this is supposed to be called sometimes (remember that we are not calling this from createSession anymore), then add a more meaningful comment for ensure that we are at the base class and are returning always false.

[Enrique Ocaña]

(In reply to comment #7)

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1148
> > +    if (!supportsKeySystem(keySystem, emptyString()))
>
> If it is not compulsory to override it and this is still correctly called
> from somewhere else, then just return nullptr, GST_DEBUG to make clear that
> we are at the base class returning always null and do not ever call
> supportsKeySystem.

supportsKeySystem() can't be removed. It's used from MediaPlayerPrivateGStreamer::registerMediaEngine(). It's part of the group of functions used by MediaPlayer to choose the right MediaPlayerPrivate implementor for each kind of media content.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1161
> > +    GST_DEBUG("Checking for KeySystem support with %s and type %s", keySystem.utf8().data(), mimeType.utf8().data());
> 
> Something similar applies here.
> 
> If this is supposed to be compulsory overridden, then ASSERT_NOT_REACHED and
> return false

Not the case, it's not to be overriden.

> If this is supposed to be called sometimes (remember that we are not calling
> this from createSession anymore), then add a more meaningful comment for
> ensure that we are at the base class and are returning always false.

There's no need to state that the comment comes from the base class. The GStreamer log system is already explicit enough about that, printing the source code file name and line number.

[Enrique Ocaña]

Created attachment 292258 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #8)
> supportsKeySystem() can't be removed. It's used from
> MediaPlayerPrivateGStreamer::registerMediaEngine(). It's part of the group
> of functions used by MediaPlayer to choose the right MediaPlayerPrivate
> implementor for each kind of media content.

Sorry for not being clearer here. I didn't mean to remove the function but the function call as you did in your next patch.

[Xabier Rodríguez Calvar]

Comment on attachment 292258 [details]
Patch

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

For me this can be fixed at landing time unless Žan has something else to say.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1145
> +    GST_DEBUG("Requested CDMSession for KeySystem %s: Returning null.", keySystem.utf8().data());

IMHO, already returning null is a weird enough situation so that we raise the debug level at least to GST_INFO.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1158
> +    GST_DEBUG("Checking for KeySystem support with %s and type %s: false.", keySystem.utf8().data(), mimeType.utf8().data());

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:125
> +    void needKey(RefPtr<Uint8Array> initData);

Remove the parameter name

[Enrique Ocaña]

Created attachment 292412 [details]
Patch

[Enrique Ocaña]

Created attachment 292897 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292897 [details]
Patch

Clearing flags on attachment: 292897

Committed r207884: <http://trac.webkit.org/changeset/207884>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.