One option considered is to make the ColorSpace field optional, see [1]. However, as pointed out by hubbe@ on the CL, that conveys the wrong intent.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1241846

A cleaner solution is to migrate the [native] struct gfx.mojom.ColorSpace to a regular Mojo struct.

My understanding is that that would expose the innnards of ui::gfx::ColorSpace which we do not want to do.

re #4: I don't understand the context of the concern. To who would this expose the innards of the class that we want to hide them from? 

ColorSpace is currently being transported across process boundaries via the legacy IPC mechanism. To this end, the ColorSpace class friends the serialization logic in order to give it access to its private fields.

When migrating to Mojo, we would be doing the same thing. I actually have the CL ready for that, see [1].

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1249092

My understanding was that the problem here is that VideoFrameInfo is being communicated to something in chromeos that doesn't rely on chrome sources. If that something tries to manipulate the data fields directly, we have a problem I think.

Maybe I'm completely missing what's going on here.

I think your understanding is correct. ChromeOS clients do not know the class ui::gfx::ColorSpace, so for them the ColorSpace arriving via Mojo (for video capture) or being sent to Mojo (for virtual devices) will not be mapped to class ui::gfx::ColorSpace. Instead the raw fields as seen here [1] will be interacted with directly.

If we want ChromeOS clients of the video capture service to understand and fill ColorSpace information, that seems unavoidable. What is the concern here?

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1249092/3/ui/gfx/mojo/color_space.mojom#77

So what happens when we change ui::gfx::ColorSpace?
Today we can do so whenever, as long as the exposed functions stays the same.
Exposing the innards of ui::gfx::ColorSpace may be the right solution for this problem, but it's neither unavoidable nor desired.

As far as I understand, this problem wasn't caused by migrating to mojo, but caused by me adding an additional field to a struct, and I wasn't aware that that struct was being used outside of chrome, so there is no legacy code for this.

When we change ui::gfx::ColorSpace then the serialization and deserialization code must be updated. That is regardless of whether or not we use legacy IPC or Mojo.

The ChromeOS clients of the video capture service API will have the additional inconvenience of having to update their usage of ColorSpace as well, even if the public API of the Chromium class ui::gfx::ColorSpace did not change. Of course, they could choose to just duplicate the code from ui::gfx::ColorSpace from Chromium, but there will be work nevertheless.

Please let me know if you see any alternative options we should explore.

Well, the first question I have is why VideoFrameInfo exists at all. The color space is already a part of the VideoFrame, so why do we need a VideoFrameInfo?

I think the answer to that question is that it's the mojo-encoded video frame is not easy to use outside of chrome, so we invented another way to send video frames over an IPC channel. I think the same is true for color spaces.

That is a very interesting question to discuss, but it might be controversial.

I introduced VideoFrameInfo while migrating the Browser-side video capture stack to a Mojo component, which is now the video capture service. The reason why I chose to not use media::VideoFrame or its Mojo equivalent is actually unrelated to usage outside of Chrome, as there was no such usage back then. The reason was that as a new reader of the code, I found the API for frame transport across IPC boundaries was very confusing. In some method calls, media::VideoFrame was used to pre-share buffers between processes. In other method calls, media::VideoFrame was used to indicate that frame data was ready to be consumed in a pre-shared buffer and to send along metadata. Then, consumers of media::VideoFrame were supposed to overwrite some metadata to send information (about load/utilization/pressure) which a DestructionObserver would eventually collect and send back to the producer of the frame.

I wanted make it clear from the API that sharing buffers is done separately from sending events about frames ready for consumption. That is why I introduced VideoFrameInfo, i.e. to separate information about buffers from information about the actual video frame content.

I realize that this has created some redundancy with what media::VideoFrame could in theory be used for, but I believe the increase in clarity has been worth it.


Going back to ColorSpace, I agree with your statement from the CL from #2 that we always want to be explicit about ColorSpace when sending around video frames. Therefore, instead of making it optional as proposed in #2, I think it is better to require frame producers outside of Chrome to provide this information as well. It does not really matter how we ask them to encode this information, but using a Mojo equivalent of ui::gfx::ColorSpace seems like the natural choice.

Right now there are several places within chrome where VideoFrameInfo is either copied into our out of a video frame. We might want to centralize those so that next time we have to add something to VideoFrameInfo, we don't have to change many places. (Or potentially miss some.)

Re #3:

> A cleaner solution is to migrate the [native] struct gfx.mojom.ColorSpace
> to a regular Mojo struct.

This is an option (it previously was a bit hairy because of some expected side-effects of creating a gfx::Colorspace). I would recommend it.

Note that the option described in #2, https://chromium-review.googlesource.com/c/chromium/src/+/1241846, has landed for now in order to get this mitigated for M71.

The change to migrate gfx::ColorSpace to mojo is still underway and might not make it into M71 anymore.

I created Bug 893203 to track the migration of gfx::ColorSpace.