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

Frank Liberato (Gerrit)

unread,
Dec 3, 2024, 1:54:07 PMDec 3
to Guido Urdaneta, Dominik Röttsches, Khushal Sagar, Chromium IPC Reviews, Dale Curtis, 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 Chromium IPC Reviews, Dominik Röttsches, Guido Urdaneta and Khushal Sagar

Frank Liberato added 5 comments

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

dalecurtis@: thanks!
guidou@: PTAL @ web_media_player_ms.*
drott@: PTAL @ web_surface_layer_bridge
khushal@: PTAL @ cc/layers
IPC: PTAL @ mojom

-fl

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

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

Frank Liberato

Done

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

pushes

Frank Liberato

Done

Line 850, Patchset 31: // not much to do.
Dale Curtis . resolved

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

Frank Liberato

Done

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

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

Frank Liberato

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Chromium IPC Reviews
  • Dominik Röttsches
  • Guido Urdaneta
  • Khushal Sagar
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: 33
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Chromium IPC Reviews <chrome-ip...@google.com>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Dominik Röttsches <dr...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
Gerrit-Reviewer: Khushal Sagar <khusha...@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: Chromium IPC Reviews <chrome-ip...@google.com>
Gerrit-Attention: Khushal Sagar <khusha...@chromium.org>
Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
Gerrit-Comment-Date: Tue, 03 Dec 2024 18:53:53 +0000
satisfied_requirement
open
diffy

gwsq (Gerrit)

unread,
Dec 3, 2024, 2:00:53 PMDec 3
to Chromium IPC Reviews, Mustafa Emre Acer, Guido Urdaneta, Dominik Röttsches, Khushal Sagar, Dale Curtis, 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 Dominik Röttsches, Guido Urdaneta, Khushal Sagar and Mustafa Emre Acer

Message from gwsq

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)

Open in Gerrit

Related details

Attention is currently required from:
  • Dominik Röttsches
  • Guido Urdaneta
  • Khushal Sagar
  • Mustafa Emre Acer
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement 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: 33
Gerrit-Owner: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Dale Curtis <dalec...@chromium.org>
Gerrit-Reviewer: Dominik Röttsches <dr...@chromium.org>
Gerrit-Reviewer: Frank Liberato <libe...@chromium.org>
Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
Gerrit-Reviewer: Khushal Sagar <khusha...@chromium.org>
Gerrit-Reviewer: Mustafa Emre Acer <mea...@chromium.org>
Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>
Gerrit-CC: Chromium IPC Reviews <chrome-ip...@google.com>
Gerrit-CC: gwsq
Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>
Gerrit-Attention: Mustafa Emre Acer <mea...@chromium.org>
Gerrit-Attention: Khushal Sagar <khusha...@chromium.org>
Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
Gerrit-Comment-Date: Tue, 03 Dec 2024 19:00:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
open
diffy

Dale Curtis (Gerrit)

unread,
Dec 3, 2024, 2:06:35 PMDec 3
to Chromium IPC Reviews, Mustafa Emre Acer, Guido Urdaneta, Dominik Röttsches, Khushal Sagar, 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 Dominik Röttsches, Frank Liberato, Guido Urdaneta, Khushal Sagar and Mustafa Emre Acer

Dale Curtis voted and added 1 comment

Votes added by Dale Curtis

Code-Review+1

1 comment

File cc/layers/surface_layer.cc
Line 214, Patchset 33 (Latest): const bool bounds_changed = bounds() != size;
Dale Curtis . unresolved

Do we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?

Open in Gerrit

Related details

Attention is currently required from:
  • Dominik Röttsches
  • Frank Liberato
  • Guido Urdaneta
  • Khushal Sagar
  • Mustafa Emre Acer
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Gerrit-Attention: Frank Liberato <libe...@chromium.org>
      Gerrit-Attention: Khushal Sagar <khusha...@chromium.org>
      Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Tue, 03 Dec 2024 19:06:24 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Khushal Sagar (Gerrit)

      unread,
      Dec 3, 2024, 4:10:35 PMDec 3
      to Chromium IPC Reviews, Mustafa Emre Acer, Guido Urdaneta, Dominik Röttsches, Dale Curtis, 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 Dominik Röttsches, Frank Liberato, Guido Urdaneta and Mustafa Emre Acer

      Khushal Sagar added 1 comment

      File cc/layers/surface_layer.h
      Line 43, Patchset 33 (Latest): using BoundsCallback = base::RepeatingCallback<void(const gfx::Size&)>;
      Khushal Sagar . unresolved

      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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Dominik Röttsches
      • Frank Liberato
      • Guido Urdaneta
      • Mustafa Emre Acer
      Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Tue, 03 Dec 2024 21:10:25 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Frank Liberato (Gerrit)

      unread,
      Dec 3, 2024, 4:28:51 PMDec 3
      to Chromium IPC Reviews, Mustafa Emre Acer, Guido Urdaneta, Dominik Röttsches, Khushal Sagar, Dale Curtis, 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 Dale Curtis, Dominik Röttsches, Guido Urdaneta and Mustafa Emre Acer

      Frank Liberato added 1 comment

      File cc/layers/surface_layer.cc
      Line 214, Patchset 33 (Latest): const bool bounds_changed = bounds() != size;
      Dale Curtis . unresolved

      Do we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?

      Frank Liberato

      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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Dale Curtis
      • Dominik Röttsches
      Gerrit-Attention: Dale Curtis <dalec...@chromium.org>
      Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Tue, 03 Dec 2024 21:28:39 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Dale Curtis <dalec...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Khushal Sagar (Gerrit)

      unread,
      Dec 3, 2024, 5:48:47 PMDec 3
      to Chromium IPC Reviews, Mustafa Emre Acer, Guido Urdaneta, Dominik Röttsches, Dale Curtis, 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 Dale Curtis, Dominik Röttsches, Frank Liberato, Guido Urdaneta and Mustafa Emre Acer

      Khushal Sagar added 1 comment

      File cc/layers/surface_layer.cc
      Line 214, Patchset 33 (Latest): const bool bounds_changed = bounds() != size;
      Dale Curtis . resolved

      Do we need to worry about the initial state here? I.e., should this be `bounds_changed || never_sent_bounds`?

      Frank Liberato

      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.

      Khushal Sagar

      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.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Dale Curtis
      • Dominik Röttsches
      • Frank Liberato
      Gerrit-Attention: Frank Liberato <libe...@chromium.org>
      Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Tue, 03 Dec 2024 22:48:31 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Dale Curtis <dalec...@chromium.org>
      Comment-In-Reply-To: Frank Liberato <libe...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Guido Urdaneta (Gerrit)

      unread,
      Dec 5, 2024, 9:46:43 AMDec 5
      to Chromium IPC Reviews, Mustafa Emre Acer, Dominik Röttsches, Khushal Sagar, Dale Curtis, 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 Dominik Röttsches, Frank Liberato and Mustafa Emre Acer

      Guido Urdaneta voted and added 1 comment

      Votes added by Guido Urdaneta

      Code-Review+1

      1 comment

      Patchset-level comments
      Guido Urdaneta . resolved

      mediastream lgtm

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Dominik Röttsches
      • Frank Liberato
      • Mustafa Emre Acer
      Gerrit-Attention: Frank Liberato <libe...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Thu, 05 Dec 2024 14:46:28 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Mustafa Emre Acer (Gerrit)

      unread,
      Dec 5, 2024, 4:13:33 PMDec 5
      to Guido Urdaneta, Chromium IPC Reviews, Dominik Röttsches, Khushal Sagar, Dale Curtis, 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 Dominik Röttsches and Frank Liberato

      Mustafa Emre Acer voted and added 1 comment

      Votes added by Mustafa Emre Acer

      Code-Review+1

      1 comment

      Patchset-level comments
      Mustafa Emre Acer . resolved

      Mojo lgtm

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Dominik Röttsches
      • Frank Liberato
      Gerrit-Attention: Frank Liberato <libe...@chromium.org>
      Gerrit-Attention: Dominik Röttsches <dr...@chromium.org>
      Gerrit-Comment-Date: Thu, 05 Dec 2024 21:13:21 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages