[webkit-reviews] review requested: [Bug 185948] [GStreamer] Handle changes in the "drm-preferred-decryption-system-id" NEED_CONTEXT message. : [Attachment 341282] (In reply to Xabier Rodríguez Calvar from comment #7)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 25 07:59:44 PDT 2018
Thibault Saunier <tsaunier at gnome.org> has asked for review:
Bug 185948: [GStreamer] Handle changes in the
"drm-preferred-decryption-system-id" NEED_CONTEXT message.
https://bugs.webkit.org/show_bug.cgi?id=185948
Attachment 341282: (In reply to Xabier Rodríguez Calvar from comment #7)
https://bugs.webkit.org/attachment.cgi?id=341282&action=review
--- Comment #9 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 341282
--> https://bugs.webkit.org/attachment.cgi?id=341282&action=review
(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 341271 [details]
> Added a new patch to GStreamer (that I will merge once this one is accepted)
> that moves back to previous behaviour
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341271&action=review
>
> > Source/WebCore/ChangeLog:3
> > + [GStreamer] Handle changes in the
"drm-preferred-decryption-system-id" NEED_CONTEXT message.
>
> This line should reflect the bug title. If you want to keep this informative
> text I'd suggest to move it down to the long description section.
Updated bug title.
> > Source/WebCore/ChangeLog:12
> > + Some tests already exist and where failling (crashing) without
that patch.
>
> I would list the tests here, something like:
> Tests: imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-*
Done.
> Side comment: "where failling" -> "were failing"
>
> > Tools/ChangeLog:3
> > + [GStreamer] Handle changes in the
"drm-preferred-decryption-system-id" NEED_CONTEXT message.
>
> Ditto.
Done.
> > Tools/ChangeLog:10
> > + properly taken into account in the qtdemux patch that uses it and
got upstreamed. Those 3 new patches fixes that and changes the
>
> ... Those 3 new patches fix that and change ...
>
> > Tools/gstreamer/jhbuild.modules:73
> > + <patch
file="gst-plugins-good-0002-qtdemux-Do-not-unref-a-NULL-stream_tags.patch"
strip="1" /> <!-- Merged as 43a540b1cd9f162d3dae5d50e36703dfaf558a3e -->
>
> I don't understand why this patch is needed here. Besides, it looks like we
> are missing
> Tools/gstreamer/patches/gst-plugins-good-0003-qtdemux-Clarify-field-name-
> about-stream-encryption-s.patch
I added the missing one (forgot to `git add` this file, sorry), The second one
is needed so that the patches are the same as upstream (this patch has been
merged already).
Basically I think that we should try to have exactly the same patches as what
is merged upstream to avoid this kind of issues when rebasing ;)
(In reply to Yacine Bandou from comment #8)
> IMHO, we don't need to use
> "gst_protection_filter_systems_by_available_decryptors" when we dispatch the
> GstContext, we simply send all the systemIDs present in the media to the
> application and let it select any systemID, the application know if this
> system is supported or not.
The thing is that maybe in WebKit we know but it might not be the case in other
apps, I think it makes the API sensibly simpler to use.
More information about the webkit-reviews
mailing list