Add Picture-in-Picture size constraint metrics and update size to DIP [chromium/src : main]
0 views
Skip to first unread message
Benjamin Keen (Gerrit)
Jun 22, 2026, 8:29:25 PM (2 days ago) Jun 22
to Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Joey Arhar and Tommy Steimel
Benjamin Keen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Benjamin Keen . resolved
Adding:
* @ste...@chromium.org for:
* `c/b/p/auto_picture_in_picture_tab_helper_browsertest.cc`
* @jar...@chromium.org for:
* `t/b/r/c/h/m/html_video_element.*`
Related details
Attention is currently required from:
- Joey Arhar
- Tommy Steimel
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Jun 22, 2026, 8:41:51 PM (2 days ago) Jun 22
to Benjamin Keen, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Benjamin Keen and Tommy Steimel
Joey Arhar voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Benjamin Keen
- Tommy Steimel
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Jun 23, 2026, 3:10:05 PM (2 days ago) Jun 23
to Benjamin Keen, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Benjamin Keen
Tommy Steimel added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 677, Patchset 2 (Latest):
if (visual_size.width() <= 0xFFFF && visual_size.width() >= 0 &&Tommy Steimel . unresolved
Are we going to be able to meaningfully analyze this encoded data? Is there precedent for logging sizes in histograms?
Related details
Attention is currently required from:
- Benjamin Keen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Jun 23, 2026, 3:28:29 PM (2 days ago) Jun 23
to Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Tommy Steimel
Benjamin Keen added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 677, Patchset 2 (Latest):
if (visual_size.width() <= 0xFFFF && visual_size.width() >= 0 &&Tommy Steimel . unresolved
Are we going to be able to meaningfully analyze this encoded data? Is there precedent for logging sizes in histograms?
Benjamin Keen
Are we going to be able to meaningfully analyze this encoded data?
I believe so. This should help us adjust the threshold to a value that captures the majority of cases where we wouldn't want to ChAP.
Is there precedent for logging sizes in histograms?
Yes, at least one example [here](https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/client_side_detection_host.cc;l=1744-1749;drc=f0d02a65a8f20891420949db8de824cf0b774687), which is where I got the idea from.
Related details
Attention is currently required from:
- Tommy Steimel
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Jun 23, 2026, 4:25:15 PM (2 days ago) Jun 23
to Benjamin Keen, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Benjamin Keen
Tommy Steimel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tommy Steimel
| Code-Review | +1 |
1 comment
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 677, Patchset 2 (Latest):
if (visual_size.width() <= 0xFFFF && visual_size.width() >= 0 &&Tommy Steimel . resolved
Benjamin KeenAre we going to be able to meaningfully analyze this encoded data? Is there precedent for logging sizes in histograms?
Are we going to be able to meaningfully analyze this encoded data?
I believe so. This should help us adjust the threshold to a value that captures the majority of cases where we wouldn't want to ChAP.
Is there precedent for logging sizes in histograms?
Yes, at least one example [here](https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/browser/client_side_detection_host.cc;l=1744-1749;drc=f0d02a65a8f20891420949db8de824cf0b774687), which is where I got the idea from.
Attention is currently required from:
- Benjamin Keen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Jun 23, 2026, 4:38:22 PM (2 days ago) Jun 23
to Takashi Toyoshima, Chromium IPC Reviews, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Chromium IPC Reviews and Takashi Toyoshima
Benjamin Keen added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Benjamin Keen . resolved
Adding @toyo...@chromium.org for: `tools/metrics/histograms/metadata/media/.*`
Related details
Attention is currently required from:
- Chromium IPC Reviews
- Takashi Toyoshima
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Jun 23, 2026, 4:41:53 PM (2 days ago) Jun 23
to Benjamin Keen, Chromium IPC Reviews, Giovanni Ortuno Urquidi, Takashi Toyoshima, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Giovanni Ortuno Urquidi and Takashi Toyoshima
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org, toyo...@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): ort...@chromium.org, toyo...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
Attention is currently required from:
- Giovanni Ortuno Urquidi
- Takashi Toyoshima
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Jun 24, 2026, 2:51:00 AM (24 hours ago) Jun 24
to Benjamin Keen, Chromium IPC Reviews, Giovanni Ortuno Urquidi, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Benjamin Keen and Giovanni Ortuno Urquidi
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Benjamin Keen
- Giovanni Ortuno Urquidi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Jun 24, 2026, 11:17:12 AM (15 hours ago) Jun 24
to Benjamin Keen, Giovanni Ortuno Urquidi, Takashi Toyoshima, Chromium IPC Reviews, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Attention needed from Benjamin Keen
Giovanni Ortuno Urquidi voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Benjamin Keen
Benjamin Keen (Gerrit)
Jun 24, 2026, 11:32:16 AM (15 hours ago) Jun 24
to Giovanni Ortuno Urquidi, Takashi Toyoshima, Chromium IPC Reviews, Tommy Steimel, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Chromium LUCI CQ (Gerrit)
Jun 24, 2026, 12:55:50 PM (14 hours ago) Jun 24
to Benjamin Keen, Giovanni Ortuno Urquidi, Takashi Toyoshima, Chromium IPC Reviews, Tommy Steimel, android-bu...@system.gserviceaccount.com, Chromium Metrics Reviews, chromium...@chromium.org, srirama chandra sekhar, asvitkine...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, ipc-securi...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Add Picture-in-Picture size constraint metrics and update size to DIP
This CL introduces three new UMA histograms to track Picture-in-Picture
size constraint outcomes:
* Media.PictureInPicture.SizeConstraintResult
* Media.PictureInPicture.AllowedVideoEncodedSize
* Media.PictureInPicture.BlockedVideoEncodedSize
The allowed and blocked video dimensions are encoded into a single
32-bit value, to correlate both width and height in a single histogram
sample. This will enable us to analyze the dimensions of both,
successful and blocked enter PiP video requests.
Additionally, this change updates the video size calculation to
correctly handle the monitor's device scale factor (DSF).
Bug: 408502322
Change-Id: Ife8148890ba1e054b30e20aff0d3fb64006fda2f
Reviewed-by: Tommy Steimel <ste...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Commit-Queue: Benjamin Keen <bk...@google.com>
Reviewed-by: Giovanni Ortuno Urquidi <ort...@chromium.org>
Reviewed-by: Joey Arhar <jar...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1651788}
Files:
- M chrome/browser/picture_in_picture/auto_picture_in_picture_tab_helper_browsertest.cc
- M media/mojo/mojom/media_player.mojom
- M third_party/blink/renderer/core/html/media/html_video_element.cc
- M third_party/blink/renderer/core/html/media/html_video_element.h
- M tools/metrics/histograms/metadata/media/enums.xml
- M tools/metrics/histograms/metadata/media/histograms.xml
Change size: L
Delta: 6 files changed, 335 insertions(+), 19 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Giovanni Ortuno Urquidi, +1 by Takashi Toyoshima, +1 by Joey Arhar, +1 by Tommy Steimel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages