[vfs] Embedder-push mode for VFS [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis added 6 comments![]( "Open in Gerrit")
012345678901234567890123456789012345678901234567890123456789012345678901100100011001010110110001100101011101000110010100111111
static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?
const media::VideoDecoderConfig& inital_config,Since you only need natural_size and transform, I'd probably just pass those in instead.
const gfx::Size& untransformed_surface_size);Fine as is, but maybe `pre_transform_surface_size`, `raw_surface_size`, or `surface_size_without_transform`? Keep it consistent everywhere if you change it.
static const base::Feature& EmbedderPushFeatureForTesting();Instead of this, probably it should go in RuntimEnabledFeatures if you need it outside of the .cc file.
// what happens if this fails to send the frame to the compositor? shouldYes, it's best effort and we can't really hold off until we get the ack or it'll be too late. rVFC is already very time sensitive.
Related details
- Frank Liberato
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 6 comments![]( "Open in Gerrit")
0123456789012345678901234567890123456789012345678901234567890123456789Frank Liberato01100100011001010110110001100101011101000110010100111111
whoops. so happy about the description i forgot to remove it.
static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?
done.
note that it means that SurfaceLayerBridge gets OnBounds callbacks even if it don't need them. it doesn't know by the time it creates the SurfaceLayer if it will have a surface id client, since wmpi creates the SurfaceLayer while VFS registers the client. this could (maybe) be fixed, but out of scope.
Since you only need natural_size and transform, I'd probably just pass those in instead.
Done
Fine as is, but maybe `pre_transform_surface_size`, `raw_surface_size`, or `surface_size_without_transform`? Keep it consistent everywhere if you change it.
went with `pre_transform_surface_size` here and in vfs.
static const base::Feature& EmbedderPushFeatureForTesting();Instead of this, probably it should go in RuntimEnabledFeatures if you need it outside of the .cc file.
Done
// what happens if this fails to send the frame to the compositor? shouldYes, it's best effort and we can't really hold off until we get the ack or it'll be too late. rVFC is already very time sensitive.
- Dale Curtis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 11 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
11 comments
BoundsCallback bounds_callback_;`ProtectedSequenceWritable<>` ?
using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;Needs some documentation similar to `UpdateSubmissionStateCB` above. Should this go alongside the def above or remain nested? Or should both be nested?
static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);Frank LiberatoShould we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?
done.
note that it means that SurfaceLayerBridge gets OnBounds callbacks even if it don't need them. it doesn't know by the time it creates the SurfaceLayer if it will have a surface id client, since wmpi creates the SurfaceLayer while VFS registers the client. this could (maybe) be fixed, but out of scope.
Do you think this is an issue? I was assuming any inconsistency here would be short lived or low frequency at least.
// Interface by the submitter to receive information about the desired"by the submitter" doesn't quite read. Can you clarify? Is this just missing "implemented by" ?
// way around. This is a temporary state; all submitters will be updated`TODO(tracking_bug): Remove after converting all submitters`
// Register a `SurfaceIdClient` with the embedder and start acceptingMojo reviewer will probably ask if the API can be designed to avoid this. I think the answer is yes, but not until all submitters are converted? If so, mark with TODO to remove.
// TODO: we don't really need `current_surface_id_`.TODO(bug)
// Description of the feature `kVideoFrameEmbedderPushesSurfaceProperties`:This probably should go alongside the runtime feature?
// (wrong) way between `VideoFrameSubmittea`r and `SurfaceLayer`. UsesFix text.
status: "stable",Probably keep this as `test` for this CL and flip in another one in case anything explodes?
Related details
- Frank Liberato
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 11 comments![]( "Open in Gerrit")
sorry about the delay. anyway, i took your advice and created a more standard embedder client rather than the weird one-off method i had before. when it's non-optional, i can remove the `?` in the mojom file but the api stays otherwise the same.
-fl
BoundsCallback bounds_callback_;Frank Liberato`ProtectedSequenceWritable<>` ?
Done
using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;Needs some documentation similar to `UpdateSubmissionStateCB` above. Should this go alongside the def above or remain nested? Or should both be nested?
nested. no idea why it wasn't.
static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);Frank LiberatoShould we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?
Dale Curtisdone.
note that it means that SurfaceLayerBridge gets OnBounds callbacks even if it don't need them. it doesn't know by the time it creates the SurfaceLayer if it will have a surface id client, since wmpi creates the SurfaceLayer while VFS registers the client. this could (maybe) be fixed, but out of scope.
Do you think this is an issue? I was assuming any inconsistency here would be short lived or low frequency at least.
it's oop canvas that will be affected -- it never registers a bounds callback because it does things slightly differently. i don't think it's a big deal since these updates are fairly infrequent.
// Interface by the submitter to receive information about the desired"by the submitter" doesn't quite read. Can you clarify? Is this just missing "implemented by" ?
completely rewrote it.
// way around. This is a temporary state; all submitters will be updated`TODO(tracking_bug): Remove after converting all submitters`
Done
// Register a `SurfaceIdClient` with the embedder and start acceptingMojo reviewer will probably ask if the API can be designed to avoid this. I think the answer is yes, but not until all submitters are converted? If so, mark with TODO to remove.
i think "all submitters are converted and the feature flag goes away". anyway, i got rid of the `SetSurfaceIdClient` call and replaced it with an optional when creating the embedder. this is probably where it should be, and just won't be optional once everything supports it.
i also renamed it to `SurfaceEmbedderClient`, because `SurfaceIdClient` was a little too specific.
// TODO: we don't really need `current_surface_id_`.Frank LiberatoTODO(bug)
stale comment, removed. i think that EmbedSurface() sets `current_surface_id_` in all the paths we care about, so i made this a local variable anyway.
// Description of the feature `kVideoFrameEmbedderPushesSurfaceProperties`:This probably should go alongside the runtime feature?
Done
// (wrong) way between `VideoFrameSubmittea`r and `SurfaceLayer`. UsesFrank LiberatoFix text.
Done
Probably keep this as `test` for this CL and flip in another one in case anything explodes?
- Dale Curtis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
4 comments
// This gets called quite a lot with the same size.In the interest of avoiding unnecessary callbacks, should these be ignored at a higher level?
// The embedder pushses the desired output size after rotation. This is thepushes
// not much to do.Do you want to DLOG a warning or DVLOG() something?
if (transform.rotation == media::VIDEO_ROTATION_90 ||Hmm, this isn't really `pre_transform_surface_size` anymore. Should we just call it size?
Related details
- Frank Liberato
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 5 comments![]( "Open in Gerrit")
dalecurtis@: thanks!
guidou@: PTAL @ web_media_player_ms.*
drott@: PTAL @ web_surface_layer_bridge
khushal@: PTAL @ cc/layers
IPC: PTAL @ mojom
-fl
In the interest of avoiding unnecessary callbacks, should these be ignored at a higher level?
Done
// The embedder pushses the desired output size after rotation. This is theFrank Liberatopushes
Done
Do you want to DLOG a warning or DVLOG() something?
Done
Hmm, this isn't really `pre_transform_surface_size` anymore. Should we just call it size?
it really is still the pretransform size -- `surface_size_` is transformed, and this code un-transforms it back to what it was. yeah, messy. updated comments to make this clearer.
Related details
- Chromium IPC Reviews
- Dominik Röttsches
- Guido Urdaneta
- Khushal Sagar
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mea...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mea...@chromium.org
Reviewer source(s):
mea...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dominik Röttsches
- Guido Urdaneta
- Khushal Sagar
- Mustafa Emre Acer
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
const bool bounds_changed = bounds() != size;Do we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?
Related details
- Dominik Röttsches
- Frank Liberato
- Guido Urdaneta
- Khushal Sagar
- Mustafa Emre Acer
Code-Coverage
Code-Review
No-Unresolved-Comments
Khushal Sagar (Gerrit)
Khushal Sagar added 1 comment![]( "Open in Gerrit")
using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;I don't think this will give you the desired size. The size of the quad also includes the device scale factor, see here: https://source.chromium.org/chromium/chromium/src/+/main:cc/layers/surface_layer_impl.cc;l=195;drc=4270363a2d1f227f804b586be7c72b23d045cb49. It's possible this happens to work because we have use zoom-for-dsf in Blink now..?
We could send this back from the compositor to main thread? It seems brittle otherwise. There's interfaces back to blink like RenderFrameMetadataProvider to hook that info back.
Related details
- Dominik Röttsches
- Frank Liberato
- Guido Urdaneta
- Mustafa Emre Acer
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
const bool bounds_changed = bounds() != size;Do we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?
i don't think so, since the callback is set in the ctor now but bounds are set explicitly later. leaving open for @khushalsagar in case i'm missing something here.
Related details
- Dale Curtis
- Dominik Röttsches
Khushal Sagar (Gerrit)
Khushal Sagar added 1 comment![]( "Open in Gerrit")
const bool bounds_changed = bounds() != size;Frank LiberatoDo we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?
i don't think so, since the callback is set in the ctor now but bounds are set explicitly later. leaving open for @khushalsagar in case i'm missing something here.
We'll start off with an empty bounds, in which case no quads are being produced so the video is not being drawn. So shouldn't be an issue.
Related details
- Dale Curtis
- Dominik Röttsches
- Frank Liberato
Guido Urdaneta (Gerrit)
Guido Urdaneta voted and added 1 comment![]( "Open in Gerrit")
Votes added by Guido Urdaneta
| Code-Review | +1 |
1 comment
- Dominik Röttsches
- Frank Liberato
- Mustafa Emre Acer
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
1 comment
Mojo lgtm
Related details
- Dominik Röttsches
- Frank Liberato