Closed Bug 1381720 Opened 6 years ago Closed 6 years ago

[EME] Update content_decryption_module.h

Categories

(Core :: Audio/Video: GMP, enhancement, P3)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(2 files, 1 obsolete file)

The content_decryption_module.h header in chromium tip has a number of changes that we don't have, namely:

* Support for cdm::ContentDecryptionModule_9 and cdm::Host_9 which add querying key status under various HDCP states, a storage ID.
* Removal of obsolete cdm interface version 7.
* Addition of VMP headers.
* Addition of 10 and 12 bit video formats.

We should update our headers.
Comment on attachment 8887307 [details]
Bug 1381720 - Update content_decryption_module.h.

https://reviewboard.mozilla.org/r/158116/#review163292

r+ with maybe-nits:

::: dom/media/gmp/widevine-adapter/content_decryption_module.h:228
(Diff revision 1)
> +// Values are chosen to be consistent with Chromium's VideoPixelFormat values.
>  enum VideoFormat {
>    kUnknownVideoFormat = 0,  // Unknown format value. Used for error reporting.
> -  kYv12,  // 12bpp YVU planar 1x1 Y, 2x2 VU samples.
> -  kI420  // 12bpp YVU planar 1x1 Y, 2x2 UV samples.
> +  kYv12 = 1,                // 12bpp YVU planar 1x1 Y, 2x2 VU samples.
> +  kI420 = 2,                // 12bpp YUV planar 1x1 Y, 2x2 UV samples.

Looking at Chromium's VideoPixelFormat, they have I420=1 and YV12=2, but we have the reverse here!
(If that's intended, please update the comment to note the exception.)

::: dom/media/gmp/widevine-adapter/content_decryption_module.h:234
(Diff revision 1)
> +  // In the following formats, each sample uses 16-bit in storage, while the
> +  // sample value is stored in the least significant N bits where N is
> +  // specified by the number after "P". For example, for YUV420P9, each Y, U,
> +  // and V sample is stored in the least significant 9 bits in a 2-byte block.
> +  kYUV420P9 = 16,
> +  kYUV420P10 = 17,
> +  kYUV422P9 = 18,
> +  kYUV422P10 = 19,
> +  kYUV444P9 = 20,
> +  kYUV444P10 = 21,
> +  kYUV420P12 = 22,
> +  kYUV422P12 = 23,
> +  kYUV444P12 = 24,

None of these new values are used in this patch.
Why have them here? (Guessing: Preparing for HDR?)

Why not have the other values 3-15, etc.?

::: dom/media/gmp/widevine-adapter/content_decryption_module.h:421
(Diff revision 1)
> +  kHdcpVersion2_0,
> +  kHdcpVersion2_1,
> +  kHdcpVersion2_2
> +};
> +
> +struct Policy {

This line (and others) tells me you may not have run clang-format. Intentional?
Attachment #8887307 - Flags: review?(gsquelart) → review+
Comment on attachment 8887308 [details]
Bug 1381720 - Add content_decryption_module_ext.h.

https://reviewboard.mozilla.org/r/158118/#review163296

::: commit-message-c6f05:5
(Diff revision 1)
> +Bug 1381720 - Add content_decryption_module_ext.h. r?gerald
> +
> +Updates to Chromium revision 6e4c388c0117fe408b66fbede91081fb1018c5fe.
> +
> +Adds Verified Media Pipeline funcction definitions.

"funcction" -> "function"
Attachment #8887308 - Flags: review?(gsquelart) → review+
Comment on attachment 8887307 [details]
Bug 1381720 - Update content_decryption_module.h.

https://reviewboard.mozilla.org/r/158116/#review163292

> Looking at Chromium's VideoPixelFormat, they have I420=1 and YV12=2, but we have the reverse here!
> (If that's intended, please update the comment to note the exception.)

Hmmm, now I see you've just copied the files as-is, so I guess my comments don't apply -- unless Chromium is actually wrong?
Comment on attachment 8887307 [details]
Bug 1381720 - Update content_decryption_module.h.

https://reviewboard.mozilla.org/r/158116/#review163292

> Hmmm, now I see you've just copied the files as-is, so I guess my comments don't apply -- unless Chromium is actually wrong?

They're handling this conversion:
https://cs.chromium.org/chromium/src/media/mojo/services/mojo_cdm_allocator.cc?l=35&rcl=f60e35b4e3744f4a3834ceb5f2677fdb7bb24dd6
https://cs.chromium.org/chromium/src/media/cdm/ppapi/ppapi_cdm_adapter.cc?l=126&rcl=f60e35b4e3744f4a3834ceb5f2677fdb7bb24dd6

I don't want to change the header, to make it easier to spot changed from upstream and stay in sync.

> None of these new values are used in this patch.
> Why have them here? (Guessing: Preparing for HDR?)
> 
> Why not have the other values 3-15, etc.?

Yes, this is in preparation for updating the CDM again.

I don't want to take these out, as I don't want to change the header.

> This line (and others) tells me you may not have run clang-format. Intentional?

Yes, I want to keep the header in sync with upstream.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69f177823866
Update content_decryption_module.h. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e4d9cb334352
Add content_decryption_module_ext.h. r=gerald
Comment on attachment 8888222 [details]
Bug 1381720 - Reset symbol visibility when including content_decryption_module.h.

https://reviewboard.mozilla.org/r/159172/#review164576

This is wrong for two reasons:
- We have "automatic" wrappers like the one you created, so putting content_decryption_module.h in config/system-headers would have the same effect as your patch with only a one line change.
- The error doesn't actually entirely make sense, and clang is being misleading. The actual problem is that the forward declarations at the beginning of content_decryption_module.h are inheriting the visibility from gcc_hidden.h because they're just in the form `class Foo`, while the full declarations are `class CDM_CLASS_API Foo` and thus get the visibility from CDM_CLASS_API. That is, the following patch fixes it:

--- a/dom/media/gmp/widevine-adapter/content_decryption_module.h
+++ b/dom/media/gmp/widevine-adapter/content_decryption_module.h
@@ -73,12 +73,12 @@ CDM_API const char* GetCdmVersion();
 
 namespace cdm {
 
-class AudioFrames;
-class DecryptedBlock;
-class VideoFrame;
+class CDM_CLASS_API AudioFrames;
+class CDM_CLASS_API DecryptedBlock;
+class CDM_CLASS_API VideoFrame;
 
-class Host_8;
-class Host_9;
+class CDM_CLASS_API Host_8;
+class CDM_CLASS_API Host_9;
 
 enum Status {
   kSuccess = 0,
Attachment #8888222 - Flags: review?(mh+mozilla) → review-
Thank you Mike.
Flags: needinfo?(cpearce)
Attachment #8888222 - Attachment is obsolete: true
Landed with Mike's fix of ensuring predeclarations of classes also have CDM_CLASS_API. Will try to get that upstreamed...
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c89f2a6edabd
Update content_decryption_module.h. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0482b58c7343
Add content_decryption_module_ext.h. r=gerald
See Also: → 1415466
You need to log in before you can comment on or make changes to this bug.