1381720 - [EME] Update content_decryption_module.h

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1381720 - Update content_decryption_module.h.

Update to chromium revision 6e4c388c0117fe408b66fbede91081fb1018c5fe.

Includes cdm::ContentDecryptionModule_9 and cdm::Host_9 definitions,
HDCP definitions, and 10 and 12 bit image format definitions.

Review commit: https://reviewboard.mozilla.org/r/158116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158116/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1381720 - Add content_decryption_module_ext.h.

Updates to Chromium revision 6e4c388c0117fe408b66fbede91081fb1018c5fe.

Adds Verified Media Pipeline funcction definitions.

Review commit: https://reviewboard.mozilla.org/r/158118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158118/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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?

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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"

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8887307 [details]
Bug 1381720 - Update content_decryption_module.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158116/diff/1-2/

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8887308 [details]
Bug 1381720 - Add content_decryption_module_ext.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158118/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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 10

[Manish Goregaokar [:manishearth]]

This push busted the static analysis builds and some other stuff

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e4d9cb334352a1209d84f603ece6684e41313ae9

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed these out for build failures on linux like https://treeherder.mozilla.org/logviewer.html#?job_id=115093423&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/dab774f84155e15c894032b05dd53619af50a6f7

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1381720 - Reset symbol visibility when including content_decryption_module.h.

content_decryption_module.h is imported from the Chromium project, which does
not set its symbol visibiity to hidden by default. But we do. So if we don't
wrap content_decryption_module.h with defines that reset our default symbol
visibility for the duration of the inlcude, we end up with build errors such
as:

In file included from /home/worker/workspace/build/src/dom/media/gmp/ChromiumCDMAdapter.cpp:7:
/home/worker/workspace/build/src/obj-firefox/dist/include/content_decryption_module.h:921:7: error: visibility does not match previous declaration
  class CDM_CLASS_API Host_8 {
        ^
/home/worker/workspace/build/src/obj-firefox/dist/include/content_decryption_module.h:32:38: note: expanded from macro 'CDM_CLASS_API'
  #define CDM_CLASS_API __attribute__((visibility("default")))
                                       ^
/home/worker/workspace/build/src/config/gcc_hidden.h:6:13: note: previous attribute is here
#pragma GCC visibility push(hidden)
            ^

Review commit: https://reviewboard.mozilla.org/r/159172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159172/

Comment 13

[Mike Hommey [:glandium]]

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,

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thank you Mike.

Comment 15

[Mike Hommey [:glandium]]

FWIW, I just filed https://bugs.llvm.org/show_bug.cgi?id=33871

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8887307 [details]
Bug 1381720 - Update content_decryption_module.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158116/diff/2-3/

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8887308 [details]
Bug 1381720 - Add content_decryption_module_ext.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158118/diff/2-3/

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Landed with Mike's fix of ensuring predeclarations of classes also have CDM_CLASS_API. Will try to get that upstreamed...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c89f2a6edabd
https://hg.mozilla.org/mozilla-central/rev/0482b58c7343