[vfs] Embedder-push mode for VFS [chromium/src : main]

0 views
Skip to first unread message

Dale Curtis (Gerrit)

unread,
Nov 8, 2024, 4:57:30 PMNov 8
to chrom...@appspot.gserviceaccount.com, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, CJ DiMeglio, Stephen Chenney, blink-revi...@chromium.org, zol...@webkit.org, alexmo...@chromium.org, creis...@chromium.org, navigation...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, drott+bl...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz...@chromium.org, tommyw+w...@chromium.org
Attention needed from Frank Liberato

Dale Curtis added 6 comments

Commit Message
Line 9, Patchset 24 (Latest):0123456789012345678901234567890123456789012345678901234567890123456789
Dale Curtis . unresolved

01100100011001010110110001100101011101000110010100111111

File cc/layers/surface_layer.h
Line 42, Patchset 24 (Latest): static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);
Dale Curtis . unresolved

Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?

File third_party/blink/public/platform/media/video_frame_compositor.h
Line 102, Patchset 24 (Latest): const media::VideoDecoderConfig& inital_config,
Dale Curtis . unresolved

Since you only need natural_size and transform, I'd probably just pass those in instead.

File third_party/blink/renderer/platform/graphics/video_frame_resource_provider.h
Line 56, Patchset 24 (Latest): const gfx::Size& untransformed_surface_size);
Dale Curtis . unresolved

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.

File third_party/blink/renderer/platform/graphics/video_frame_submitter.h
Line 283, Patchset 24 (Latest): static const base::Feature& EmbedderPushFeatureForTesting();
Dale Curtis . unresolved

Instead of this, probably it should go in RuntimEnabledFeatures if you need it outside of the .cc file.

File third_party/blink/renderer/platform/media/video_frame_compositor.cc
Line 258, Patchset 24 (Latest): // what happens if this fails to send the frame to the compositor? should
Dale Curtis . unresolved

Yes, 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.

Open in Gerrit

Related details

Attention is currently required from:
  • Frank Liberato
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I077d5c89b822e914086a4bca9a48bd2c6e64dac9
Gerrit-Change-Number: 5840266
Gerrit-PatchSet: 24
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Frank Liberato <libe...@chromium.org>
Gerrit-Comment-Date: Fri, 08 Nov 2024 21:57:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Frank Liberato (Gerrit)

unread,
Nov 12, 2024, 1:49:04 PMNov 12
to Dale Curtis, chrom...@appspot.gserviceaccount.com, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, CJ DiMeglio, Stephen Chenney, jmedle...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, alexmo...@chromium.org, creis...@chromium.org, navigation...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, drott+bl...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz...@chromium.org, tommyw+w...@chromium.org
Attention needed from Dale Curtis

Frank Liberato added 6 comments

Commit Message
Line 9, Patchset 24:0123456789012345678901234567890123456789012345678901234567890123456789
Dale Curtis . resolved

01100100011001010110110001100101011101000110010100111111

Frank Liberato

whoops. so happy about the description i forgot to remove it.

File cc/layers/surface_layer.h
Line 42, Patchset 24: static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);
Dale Curtis . resolved

Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?

Frank Liberato

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.

File third_party/blink/public/platform/media/video_frame_compositor.h
Line 102, Patchset 24: const media::VideoDecoderConfig& inital_config,
Dale Curtis . resolved

Since you only need natural_size and transform, I'd probably just pass those in instead.

Frank Liberato

Done

File third_party/blink/renderer/platform/graphics/video_frame_resource_provider.h
Line 56, Patchset 24: const gfx::Size& untransformed_surface_size);
Dale Curtis . resolved

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.

Frank Liberato

went with `pre_transform_surface_size` here and in vfs.

File third_party/blink/renderer/platform/graphics/video_frame_submitter.h
Line 283, Patchset 24: static const base::Feature& EmbedderPushFeatureForTesting();
Dale Curtis . resolved

Instead of this, probably it should go in RuntimEnabledFeatures if you need it outside of the .cc file.

Frank Liberato

Done

File third_party/blink/renderer/platform/media/video_frame_compositor.cc
Line 258, Patchset 24: // what happens if this fails to send the frame to the compositor? should
Dale Curtis . resolved

Yes, 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.

Frank Liberato

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:
  • Dale Curtis
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I077d5c89b822e914086a4bca9a48bd2c6e64dac9
Gerrit-Change-Number: 5840266
Gerrit-PatchSet: 27
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Dale Curtis <dalec...@chromium.org>
Gerrit-Comment-Date: Tue, 12 Nov 2024 18:48:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Dale Curtis <dalec...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Dale Curtis (Gerrit)

unread,
Nov 12, 2024, 3:13:47 PMNov 12
to chrom...@appspot.gserviceaccount.com, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, CJ DiMeglio, Stephen Chenney, jmedle...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, alexmo...@chromium.org, creis...@chromium.org, navigation...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, drott+bl...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz...@chromium.org, tommyw+w...@chromium.org
Attention needed from Frank Liberato

Dale Curtis voted and added 11 comments

Votes added by Dale Curtis

Code-Review+1

11 comments

Patchset-level comments
File-level comment, Patchset 27 (Latest):
Dale Curtis . resolved

lgtm % comments.

File cc/layers/surface_layer.h
Line 127, Patchset 27 (Latest): BoundsCallback bounds_callback_;
Dale Curtis . unresolved

`ProtectedSequenceWritable<>` ?

Line 39, Patchset 27 (Latest): using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;
Dale Curtis . unresolved

Needs some documentation similar to `UpdateSubmissionStateCB` above. Should this go alongside the def above or remain nested? Or should both be nested?

Line 42, Patchset 24: static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);
Dale Curtis . unresolved

Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?

Frank Liberato

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.

Dale Curtis

Do you think this is an issue? I was assuming any inconsistency here would be short lived or low frequency at least.

File third_party/blink/public/mojom/frame_sinks/embedded_frame_sink.mojom
Line 15, Patchset 27 (Latest):// Interface by the submitter to receive information about the desired
Dale Curtis . unresolved

"by the submitter" doesn't quite read. Can you clarify? Is this just missing "implemented by" ?

Line 21, Patchset 27 (Latest):// way around. This is a temporary state; all submitters will be updated
Dale Curtis . unresolved

`TODO(tracking_bug): Remove after converting all submitters`

Line 48, Patchset 27 (Latest): // Register a `SurfaceIdClient` with the embedder and start accepting
Dale Curtis . unresolved

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

File third_party/blink/renderer/platform/graphics/surface_layer_bridge.cc
Line 201, Patchset 27 (Latest): // TODO: we don't really need `current_surface_id_`.
Dale Curtis . unresolved

TODO(bug)

File third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
Line 56, Patchset 27 (Latest):// Description of the feature `kVideoFrameEmbedderPushesSurfaceProperties`:
Dale Curtis . unresolved

This probably should go alongside the runtime feature?

File third_party/blink/renderer/platform/runtime_enabled_features.json5
Line 4488, Patchset 27 (Latest): // (wrong) way between `VideoFrameSubmittea`r and `SurfaceLayer`. Uses
Dale Curtis . unresolved

Fix text.

Line 4494, Patchset 27 (Latest): status: "stable",
Dale Curtis . unresolved

Probably keep this as `test` for this CL and flip in another one in case anything explodes?

Open in Gerrit

Related details

Attention is currently required from:
  • Frank Liberato
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I077d5c89b822e914086a4bca9a48bd2c6e64dac9
Gerrit-Change-Number: 5840266
Gerrit-PatchSet: 27
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-Attention: Frank Liberato <libe...@chromium.org>
Gerrit-Comment-Date: Tue, 12 Nov 2024 20:13:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Dale Curtis <dalec...@chromium.org>
Comment-In-Reply-To: Frank Liberato <libe...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Frank Liberato (Gerrit)

unread,
Nov 25, 2024, 10:54:39 PM (7 days ago) Nov 25
to srirama chandra sekhar, Dale Curtis, chrom...@appspot.gserviceaccount.com, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, CJ DiMeglio, Stephen Chenney, eric.c...@apple.com, jmedle...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, alexmo...@chromium.org, creis...@chromium.org, navigation...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, drott+bl...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz...@chromium.org, tommyw+w...@chromium.org
Attention needed from Dale Curtis

Frank Liberato added 11 comments

Patchset-level comments
File-level comment, Patchset 31 (Latest):
Frank Liberato . resolved

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

File cc/layers/surface_layer.h
Line 127, Patchset 27: BoundsCallback bounds_callback_;
Dale Curtis . resolved

`ProtectedSequenceWritable<>` ?

Frank Liberato

Done

Line 39, Patchset 27: using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;
Dale Curtis . resolved

Needs some documentation similar to `UpdateSubmissionStateCB` above. Should this go alongside the def above or remain nested? Or should both be nested?

Frank Liberato

nested. no idea why it wasn't.

Line 42, Patchset 24: static scoped_refptr<SurfaceLayer> Create(UpdateSubmissionStateCB);
Dale Curtis . resolved

Should we take this callback on the constructor here like we do for submission state? Callers can just create it with WeakPtr?

Frank Liberato

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.

Dale Curtis

Do you think this is an issue? I was assuming any inconsistency here would be short lived or low frequency at least.

Frank Liberato

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.

File third_party/blink/public/mojom/frame_sinks/embedded_frame_sink.mojom
Line 15, Patchset 27:// Interface by the submitter to receive information about the desired
Dale Curtis . resolved

"by the submitter" doesn't quite read. Can you clarify? Is this just missing "implemented by" ?

Frank Liberato

completely rewrote it.

Line 21, Patchset 27:// way around. This is a temporary state; all submitters will be updated
Dale Curtis . resolved

`TODO(tracking_bug): Remove after converting all submitters`

Frank Liberato

Done

Line 48, Patchset 27: // Register a `SurfaceIdClient` with the embedder and start accepting
Dale Curtis . resolved

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

Frank Liberato

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.

File third_party/blink/renderer/platform/graphics/surface_layer_bridge.cc
Line 201, Patchset 27: // TODO: we don't really need `current_surface_id_`.
Dale Curtis . resolved

TODO(bug)

Frank Liberato

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.

File third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
Line 56, Patchset 27:// Description of the feature `kVideoFrameEmbedderPushesSurfaceProperties`:
Dale Curtis . resolved

This probably should go alongside the runtime feature?

Frank Liberato

Done

File third_party/blink/renderer/platform/runtime_enabled_features.json5
Line 4488, Patchset 27: // (wrong) way between `VideoFrameSubmittea`r and `SurfaceLayer`. Uses
Dale Curtis . resolved

Fix text.

Frank Liberato

Done

Line 4494, Patchset 27: status: "stable",
Dale Curtis . resolved

Probably keep this as `test` for this CL and flip in another one in case anything explodes?

Frank Liberato

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Dale Curtis
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I077d5c89b822e914086a4bca9a48bd2c6e64dac9
Gerrit-Change-Number: 5840266
Gerrit-PatchSet: 31
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
Gerrit-Attention: Dale Curtis <dalec...@chromium.org>
Gerrit-Comment-Date: Tue, 26 Nov 2024 03:54:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Dale Curtis (Gerrit)

unread,
Nov 26, 2024, 5:15:57 PM (6 days ago) Nov 26
to srirama chandra sekhar, chrom...@appspot.gserviceaccount.com, AyeAye, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, CJ DiMeglio, Stephen Chenney, eric.c...@apple.com, jmedle...@chromium.org, blink-revi...@chromium.org, zol...@webkit.org, alexmo...@chromium.org, creis...@chromium.org, navigation...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, drott+bl...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz...@chromium.org, tommyw+w...@chromium.org
Attention needed from Frank Liberato

Dale Curtis voted and added 4 comments

Votes added by Dale Curtis

Code-Review+1

4 comments

File third_party/blink/renderer/platform/graphics/surface_layer_bridge.cc
Line 188, Patchset 31 (Latest): // This gets called quite a lot with the same size.
Dale Curtis . unresolved

In the interest of avoiding unnecessary callbacks, should these be ignored at a higher level?

File third_party/blink/renderer/platform/graphics/video_frame_submitter.cc
Line 845, Patchset 31 (Latest): // The embedder pushses the desired output size after rotation. This is the
Dale Curtis . unresolved

pushes

Line 850, Patchset 31 (Latest): // not much to do.
Dale Curtis . unresolved

Do you want to DLOG a warning or DVLOG() something?

Line 1062, Patchset 31 (Latest): if (transform.rotation == media::VIDEO_ROTATION_90 ||
Dale Curtis . unresolved

Hmm, this isn't really `pre_transform_surface_size` anymore. Should we just call it size?

Open in Gerrit

Related details

Attention is currently required from:
  • Frank Liberato
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I077d5c89b822e914086a4bca9a48bd2c6e64dac9
Gerrit-Change-Number: 5840266
Gerrit-PatchSet: 31
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
Gerrit-Attention: Frank Liberato <libe...@chromium.org>
Gerrit-Comment-Date: Tue, 26 Nov 2024 22:15:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages