Closed Bug 1671246 Opened 4 years ago Closed 3 years ago

Merge paths in GMP IPC code the handle node ID as strings and structs

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1651239 merged some of our code that handles node IDs in the GMP to reduce duplication and the risk of changing one path but not the other.

We can merge some of the other parts around the IPC boundary to help reduce duplication and make the code more concise and (hopefully) understandable.

Background: Node IDs are used when fetching GMPs such that GMPs with the same node ID can be shared (note, there's some other conditions the need also be met[0]). We currently have 2 ways of storing node IDs

  1. As a string. If we know the node ID early on in GMP creation we can immediately store it in a string and not mutate it further.
  2. As parts that will later be converted to a final string[1]. For EME we want to ensure GMPs are not shared between origins, so we pass around the GMP name + the top level origin and the origin of the media element involved. We later turn this into a string that will be used as the final node ID[1].

The code for handling these 2 cases is largely similar around IPC, but for the second case when the node ID reaches the GMPServiceParent we need to ensure it's processed into a string.

I think we can unify and trim some code here. I'm not planning on making any functional changes, but aiming to reduce duplication as well as consolidating some of our data types so it's easier to find where they're being used in tools like searchfox (e.g. right now we have 2 NodeId structs NodeId and NodeIdData which are essentially the same thing).

[0] https://searchfox.org/mozilla-central/rev/9c72508fcf2bba709a5b5b9eae9da35e0c707baa/dom/media/gmp/GMPServiceParent.cpp#711,722-724,727
[1] https://searchfox.org/mozilla-central/rev/9c72508fcf2bba709a5b5b9eae9da35e0c707baa/dom/media/gmp/GMPServiceParent.cpp#1065

Passing a union here allows us to reuse code and trim some code which was
duplicated to handle the different NodeId formats. This also consolidates the
former NodeId and NodeIdData structures into a new structure (working name
NodeIdParts) which represents parts that will later be converted to a string
based NodeId.

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)

I've gotten bogged down in a few other things, but rest assured, release management bot, I have not forgotten about this.

Flags: needinfo?(bvandyk)
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65b01a8227ff
Use a union to pass NodeId in GMP. r=jbauman,ipc-reviewers,nika
Severity: -- → N/A
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: