1513026 - Rename GPU/RDD protocol classes to be more explicit

Status: NEW

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

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The PGPU protocol is upside-down, with GPUParent in the child process and GPUChild in the parent; bug 1271180 comment #8 doesn't clearly state a rationale but suggests this is because of all the sync calls from the UI process into the GPU process.

However, this is not the case for PRDD, and having the protocol backwards makes it harder to share common code (see bug 1321492 comment #3).  Also, there are some subtle assumptions that the parent can outlive the child but not the reverse (definitely the presence/absence of must-use annotations on Send methods, maybe other things), so it's generally better to follow the normal way of things when possible.

I have patches that swap/rename the classes and RDD AV1 playback still works; it looks like I'll need to redo them in Mercurial to get proper rename metadata, because Git's rename detection doesn't appear to handle rename cycles.

Comment 1

[Jean-Yves Avenard [:jya]]

What is child process vs parent process here?

There's only a single GPU process (or RDD process at this stage)

Comment 2

[Jean-Yves Avenard [:jya]]

To add.

One of our goal is to share as much code between the GPU decoder and the RDD decoder.

So this bug will go against that.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> What is child process vs parent process here?

There's a parent/child process relationship at the OS level, but also, the parent (UI) process is what caused the child (RDD, GPU, content, etc.) process to be started, and can in principle restart if it needed.


(In reply to Jean-Yves Avenard [:jya] from comment #2)
> To add.
> 
> One of our goal is to share as much code between the GPU decoder and the RDD
> decoder.
> 
> So this bug will go against that.

How much of that would be in the PRDD protocol itself?  Currently it just deals with initialization and the process-level things like memory reporting, and sets up PRemoteDecoderManager instances for everything else.

One of the IPC module's goals is trying to clean up / deduplicate the per-process-type boilerplate, and having PRDD in its current state isn't a huge problem given that we'll still have to deal with PGPU, but I thought it might help a little if there were only one special case instead of two.

Comment 4

[Jean-Yves Avenard [:jya]]

I understand what a child process is, I was referring to what definition you were referring to :)

However, here from our architecture point of view, that is irrelevant how physically the kernel sees things.

We have the RDD/GPU parent that is running in the RDD/GPU process, it's a singleton and takes queries/requests from its various children.

In this context, the use of child/parent as used by the RDD/GPU is far more explicit and provide better explanation to how things work don't you think?

:mjf opinion?

Comment 5

[Jean-Yves Avenard [:jya]]

After discussion with :mjf we're going to rename the various classes so it doesn't contain the reference Child/Parent which will be reserved to the setting up of the GPU/RDD process

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

For what it's worth, I'd been wondering if we could change the Parent/Child naming to something more generic across the board (Server/Client, Superior/Inferior, …), but that's a larger task and difficult to justify.  (Also, as long as I'm digressing, one of the ideas thrown around at the all-hands was if we could have a minimal supervisor process and make the UI process just another of its sandboxed children, like what bug 845191 proposed, but even then there's still a logical “parent” that requested the other process be launched.)

Anyway, there are two things going on here:

1. The naming of the concrete classes (RDDParent, RDDChild), which is mostly arbitrary and ideally should be what makes the most sense to the people working on the media code.

2. The direction of the protocol (the parent:/child: markers in PRDD.ipdl), which does have side-effects and is the thing I actually wanted to see if we could change.

But item #2 drags in item #1 because I don't think anyone involved would want to have a class named RDDChild that inherits from PRDDParent.

Comment 7

[Michael Froman [:mjf]]

All of this is now far more difficult after the IPC devirtualization effort from Bug 1512990. It is no longer possible to have implementations of a protocol that don't match naming convention (PProtocol implemented by ProtocolChild.{h|cpp} and ProtocolParent.{h|cpp}) unless the non-standard impl is added to ipc/ipdl/ipdl/direct_call.py. The intent is to burn that list down. Until the ipdl processing is updated to support more flexible naming, this bug will have to wait.

Comment 8

[Michael Froman [:mjf]]

Attachment: RDD-naming-refactor.patch

This was work in progress, but is no longer possible after the IPC devirtualization changes.