Summary: | [GStreamer][EME] Add EME support to base private player | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrique Ocaña <eocanha> | ||||||||||||
Component: | Platform | Assignee: | Enrique Ocaña <eocanha> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | calvaris | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 157314 | ||||||||||||||
Attachments: |
|
Description
Enrique Ocaña
2016-10-04 09:14:24 PDT
Created attachment 290605 [details]
Patch
Comment on attachment 290605 [details]
Patch
Wait until all the patches in 157314 are ready.
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. 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? 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. Created attachment 291767 [details]
Patch
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. (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. Created attachment 292258 [details]
Patch
(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. 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 Created attachment 292412 [details]
Patch
Created attachment 292897 [details]
Patch
Comment on attachment 292897 [details] Patch Clearing flags on attachment: 292897 Committed r207884: <http://trac.webkit.org/changeset/207884> All reviewed patches have been landed. Closing bug. |