Closed Bug 1654383 Opened 4 years ago Closed 4 years ago

Use encryption scheme enum from content_decryption_module.h rather than re-implementing as a GMP type

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

Details

Attachments

(1 file)

The CDM headers have a definition of encryption scheme[0] we can use rather than re-implementing our own type[1]. Historically we've used our own types, but in this case we're just adding an extra conversion step.

If we ever need more flexibility we can add back in our own type, but for now this allows us to cut some code.

[0] https://searchfox.org/mozilla-central/rev/3b6958c26049c1e27b2790a43154caaba9f6dd4a/dom/media/gmp/widevine-adapter/content_decryption_module.h#101
[1] https://searchfox.org/mozilla-central/rev/3b6958c26049c1e27b2790a43154caaba9f6dd4a/dom/media/gmp/gmp-api/gmp-video-codec.h#222

This removes the need to do some conversions and simplifies the code a little.

Drive by remove an assert which is already covered by a switch statement
containing a MOZ_ASSERT_UNREACHABLE in ChromiumCDMParent.

Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9cec7707e84d Replace GMPEncryptionType with cdm::EncryptionScheme. r=alwu,dminor

Ah, good ole X11 defines [0]. Looks like the problem is we get X11 headers which #define Status int and then when we compile the ipdl files we include the CDM headers which have an unscoped enum named Status that gets turned into enum int : uint32_t which breaks building the ipdl protocols[1].

We're able to use types from the CDM headers in other ipc[2]. I'm not 100% clear on what exactly is going on with the build. Chromium have updated their headers so the enum causing us problems here no longer exists. Re-pulling the headers may be an easier way to fix this than messing with the build. It certainly is from my perspective.

[0] https://searchfox.org/mozilla-central/source/gfx/src/X11UndefineNone.h
[1] https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/python/mozbuild/mozbuild/frontend/data.py#377
[2] https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/dom/media/gmp/PChromiumCDM.ipdl#9

Re-pulling headers may not work, as I see there exist at least one other Status enum that continues to exist after the update. I wonder if the #define trampling here but not in the other files is a product of the unified build boundaries. Going to be on PTO shortly, holding NI for when I'm back.

Suggestion to self when I return to this is to create a header that contains the CDM header, but ensures Status is undefined. We already do this in GMPUtils [0], we can just shift that into a unified build friendly header so that we can use the CDM symbols without breaking the build in different contexts. Gfx does similarly for X11 [1].

Alternatively we could find what's dragging X11 headers into the unified build and fix that. In some ways that seems more ideal, but I don't know how to quickly do that, and the above seems like a more pragmatic use of time.

[0] https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/dom/media/gmp/GMPMessageUtils.h#13
[1] https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/gfx/src/X11Util.h#15

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bryce, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(bvandyk)

Thanks bot. Been on pto, but acking the need to action this.

Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9ba10704b7e3 Replace GMPEncryptionType with cdm::EncryptionScheme. r=alwu,dminor
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Flags: needinfo?(bvandyk)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: