Implementation of Cef.RegisterWidevineCdm #1935

justinstenning · 2017-02-13T00:38:52Z

Implementation of Cef.RegisterWidevineCdm with test as per feature request #1934

I have completed testing, however in order for others to test you need to:

have access to WidevineCdm binaries (it might be possible test using Chrome copy, possibly with manifest.json changes - see Feature Request - Expose CefRegisterWidevineCdm #1934 for list of files required and example manifest.json)
uncomment the call to Cef.RegisterWidevineCdm(@".\WidevineCdm"); in CefExample.Init
place the Widevine binaries in a folder called 'WidevineCdm' within the CefSharp.Wpf.Example build directory (e.g. bin\x64\Debug\WidevineCdm)

Please advise if any changes needed, styling issues, etc...

Description of changes:

Added types CefSharp/CdmRegistration.cs (return value of registration process) and enumeration CefSharp/CdmRegistrationErrorCode.cs to CefSharp
Added RegisterWidevineCdm metho to Cef.h in CefSharp.Core
Added CefRegisterCdmCallbackWrapper.h to CefSharp.Core project

…arp#1934

…efsharp#1934

justinstenning · 2017-02-13T01:00:31Z

I have just noticed that I didn't postfix the RegisterWidevineCdm method name with "Async".

Please advise if this should be the case (it returns a Task<CdmRegistration^>^).

amaitland

Comments made inline, overall it looks promising 👍

amaitland · 2017-02-13T00:54:06Z

CefSharp.Core/Cef.h

+        /// will receive a |result| value of CEF_CDM_REGISTRATION_ERROR_NOT_SUPPORTED.
+        /// </summary>
+        /// <returns>Returns the CDM registration result.</returns>
+        static Task<CdmRegistration^>^ RegisterWidevineCdm(String^ path)


I know a similar pattern has been used in the past, I've revised my take on this, would prefer to wrap the underlying API as close to 1:1 as possible and implement any async wrappers in C#.

Please add IRegisterCdmCallback as a param and return void here.

By all means add an overload that has the same signature

Overload should be RegisterWidevineCdmAsync, you are correct.

amaitland · 2017-02-13T00:54:59Z

CefSharp.Core/Internals/CefRegisterCdmCallbackWrapper.h

+        private class CefRegisterCdmCallbackWrapper : public CefRegisterCdmCallback
+        {
+        private:
+            gcroot<TaskCompletionSource<CdmRegistration^>^> _taskCompletionSource;


Replace with IRegisterCdmCallback

Do you want a base concrete implementation of IRegisterCdmCallback as well or just implement an example within the example project?

If you wish to have Cef.RegisterWidevineCdmAsync then add a concrete implementation to the CefSharp project.

Sorry to be a pain, I'm fairly new to the CefSharp codebase and not that familiar with other areas of it.

In order to call the underlying CefRegisterWidevineCdm it wants a CefRegisterCdmCallback (defined in the Cef codebase). Can you please point me to an example where a similar process takes place but using the interface as you described?

Or are you saying pass our own defined interface into the callback wrapper and use that to trigger back through to the caller? But still use the CefRegisterCdmCallback internally.

Again, sorry for the noobness!

Never mind I get it, just replace the _taskCompletionSource with it. :)

Never mind I get it, just replace the _taskCompletionSource with it. :)

Pretty much, see https://github.com/cefsharp/CefSharp/blob/master/CefSharp.Core/Internals/CefCompletionCallbackAdapter.h for an example.

Also IRegisterCdmCallback should implement IDisposable

amaitland · 2017-02-13T00:58:16Z

CefSharp.Example/Resources/CdmSupportTest.html

+        }
+    </script>
+</head>
+<body onload="doTest()">


Should probably be some sort of disclaimer here that explains widevine CDM requires additional steps to enable and propritary codes are not supported in the default Nuget packages as they require licensing.

What happens if you open this page with a default build?

If Widevine not registered and not using proprietary codecs just says that widevine is "NOT AVAILABLE", and some of the codecs are listed as not supporting playback.

I'll add a note at the end of the page re proprietary codecs and contacting Widevine for access to widevine - and a link to the issue #1934.

amaitland · 2017-02-13T00:59:18Z

CefSharp.Example/CefExample.cs

@@ -41,6 +42,10 @@ public static void Init(bool osr, bool multiThreadedMessageLoop, IBrowserProcess
            // Environment.SetEnvironmentVariable("GOOGLE_DEFAULT_CLIENT_ID", "");
            // Environment.SetEnvironmentVariable("GOOGLE_DEFAULT_CLIENT_SECRET", "");

+            // Widevine CDM registration - pass in directory where Widevine CDM binaries and manifest.json are located.
+            // To organize access to Widevine binaries: https://www.widevine.com/contact.html


Should link to the issue #1934

…ortTest.html

…e.cs

…back parameter

justinstenning · 2017-02-13T03:47:03Z

@amaitland your review comments should now all be addressed:

added IRegisterCdmCallback and simple concrete class for convenience.
Cef.RegisterWidevineCdm async result replaced with IRegisterCdmCallback parameter. One override with no parameter still provided as often the result of the process will be of no interest - this also keeps the example simple.
CdmSupportTest.html includes links to additional setup steps for both Widevine and proprietary codecs
CefExample.cs links to issue for more details. The call to Cef.RegisterWidevineCdm is left uncommented as it has no impact if the directory does not exist, this means someone could try out the Wpf Example without recompiling and just placing the widevine package in place.

amaitland · 2017-02-13T03:50:02Z

CefSharp.Core/Cef.h

+        /// </summary>
+        static void RegisterWidevineCdm(String^ path, IRegisterCdmCallback^ callback)
+        {
+            auto adapter = new CefRegisterCdmCallbackAdapter(callback);


There's no point creating a CefRegisterCdmCallbackAdapter instance of callback is null, so add nullptr check here and remove the RegisterWidevineCdm overload.

You mean pass nullptr into CefRegisterWidevineCdm when there is no callback provided? (tested and yep that doesn't cause any issues with the CEF API - at this time anyway).

/--cef(optional_param=callback)--/

The CEF API says it's optional, so yes pass in NULL when there is no callback.

https://bitbucket.org/chromiumembedded/cef/commits/a1fc6f1#Linclude/cef_web_plugin.hT210

amaitland · 2017-02-13T03:50:34Z

CefSharp.Core/Internals/CefRegisterCdmCallbackAdapter.h

+
+            ~CefRegisterCdmCallbackAdapter()
+            {
+                if (static_cast<IRegisterCdmCallback^>(_callback) != nullptr)


You can call delete on a nullptr, ends up being a noop, so no need to check this.

amaitland · 2017-02-13T03:51:04Z

CefSharp.Core/Internals/CefRegisterCdmCallbackAdapter.h

+                r->ErrorCode = (CdmRegistrationErrorCode)result;
+                r->ErrorMessage = StringUtils::ToClr(error_message);
+
+                if (static_cast<IRegisterCdmCallback^>(_callback) != nullptr)


Better to only create an instance if there is a callback and remove this check.

amaitland · 2017-02-13T03:52:20Z

CefSharp.Core/Cef.h

+        ///
+        /// If any of these files are missing or if the manifest file has incorrect
+        /// contents the registration will fail and |callback| will receive a |result|
+        /// value of CEF_CDM_REGISTRATION_ERROR_INCORRECT_CONTENTS.


Comments should be updated to make relevant in context of CefSharp.

amaitland · 2017-02-13T03:53:49Z

CefSharp/RegisterCdmCallback.cs

+    /// <summary>
+    /// Provides a simple callback implementation of <see cref="IRegisterCdmCallback"/> for use with Widevine CDM registration.
+    /// </summary>
+    public class RegisterCdmCallback: IRegisterCdmCallback


Having the default implementation implement a TaskCompletionSource wrapper would be consistent with the rest of the codebase and allow you to add Cef.RegisterWidevineCdmAsync

amaitland · 2017-02-13T03:55:10Z

@spazzarama Few more minor changes required 👍

justinstenning · 2017-02-13T04:41:57Z

@amaitland those changes should be addressed now. Let me know if anything else needs to be sorted.

amaitland · 2017-02-13T05:56:37Z

@spazzarama Merged with the following changes ee5fd34

justinstenning added 2 commits February 13, 2017 11:26

Added Cef.RegisterWidevineCdm for CEF Widevine CDM registration cefsh…

4f7ea22

…arp#1934

Added test page for WidevineCdm registration and example registration c…

442f311

…efsharp#1934

amaitland requested changes Feb 13, 2017

View reviewed changes

justinstenning added 3 commits February 13, 2017 14:17

Noted additional steps for Widevine and proprietary codecs to CdmSupp…

eb2459e

…ortTest.html

Link for more information changed to issue cefsharp#1934 in CefExampl…

aa7de2e

…e.cs

replaced Cef.RegisterCdmCallback's async result with IRegisterCdmCall…

c554b76

…back parameter

amaitland reviewed Feb 13, 2017

View reviewed changes

RegisterCdmCallback now uses taskCompletionSource

9cb80e5

amaitland approved these changes Feb 13, 2017

View reviewed changes

amaitland merged commit 2298ea0 into cefsharp:master Feb 13, 2017

amaitland added this to the 57.0.0 milestone Feb 13, 2017

mbragg12 mentioned this pull request Apr 26, 2018

Feature Request - Expose CefRegisterWidevineCdm #1934

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Cef.RegisterWidevineCdm #1935

Implementation of Cef.RegisterWidevineCdm #1935

justinstenning commented Feb 13, 2017 •

edited

Loading

justinstenning commented Feb 13, 2017

amaitland left a comment

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

justinstenning Feb 13, 2017

amaitland Feb 13, 2017

justinstenning Feb 13, 2017

justinstenning Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

justinstenning Feb 13, 2017 •

edited

Loading

amaitland Feb 13, 2017

justinstenning commented Feb 13, 2017 •

edited

Loading

amaitland Feb 13, 2017

justinstenning Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland Feb 13, 2017

amaitland commented Feb 13, 2017

justinstenning commented Feb 13, 2017

amaitland commented Feb 13, 2017

Implementation of Cef.RegisterWidevineCdm #1935

Implementation of Cef.RegisterWidevineCdm #1935

Conversation

justinstenning commented Feb 13, 2017 • edited Loading

justinstenning commented Feb 13, 2017

amaitland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinstenning Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinstenning commented Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaitland commented Feb 13, 2017

justinstenning commented Feb 13, 2017

amaitland commented Feb 13, 2017

justinstenning commented Feb 13, 2017 •

edited

Loading

justinstenning Feb 13, 2017 •

edited

Loading

justinstenning commented Feb 13, 2017 •

edited

Loading