[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. |