Surface within DevTools AutoPiP for media playback conditions [chromium/src : main]
Tommy Steimel (Gerrit)
Tommy Steimel added 3 comments![]( "Open in Gerrit")
void OnAutoPictureInPictureInfoChanged(This should be up in the `// MediaSessionPlayerObserver implementation.` section
if (normal_players_.size() == 0) {nit: `normal_players_.empty()`
for (const auto& it : normal_players_) {Is there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 3 comments![]( "Open in Gerrit")
This should be up in the `// MediaSessionPlayerObserver implementation.` section
Done
if (normal_players_.size() == 0) {Benjamin Keennit: `normal_players_.empty()`
Done
for (const auto& it : normal_players_) {Is there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
For video conferencing it should be ok to skip surfacing this info. The conditions for AutoPiP are pretty straight forward (camera/mic permission). But I am open to include those cases as well.
Do you think we should use `ForAllPlayers` instead?
Related details
- Tommy Steimel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Tommy Steimel added 1 comment![]( "Open in Gerrit")
for (const auto& it : normal_players_) {Benjamin KeenIs there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
For video conferencing it should be ok to skip surfacing this info. The conditions for AutoPiP are pretty straight forward (camera/mic permission). But I am open to include those cases as well.
Do you think we should use `ForAllPlayers` instead?
I guess what I'm saying is this doesn't seem to be a player-specific thing, so recording it for each player doesn't seem to make sense to me, though to be fair I don't know much about media log
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
for (const auto& it : normal_players_) {Benjamin KeenIs there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
Tommy SteimelFor video conferencing it should be ok to skip surfacing this info. The conditions for AutoPiP are pretty straight forward (camera/mic permission). But I am open to include those cases as well.
Do you think we should use `ForAllPlayers` instead?
I guess what I'm saying is this doesn't seem to be a player-specific thing, so recording it for each player doesn't seem to make sense to me, though to be fair I don't know much about media log
Yes that's right. The log is not specific to a player, however media logs all seem to be assosicated to a player by design (DevTools -> Media), which is the reason I am surfacing the info for all players.
@tmath...@chromium.org: WDYT of this approach? Or is there an alternative to add this information to the Media DevTools section w/o using the players?
Related details
- Ted (Chromium) Meyer
- Tommy Steimel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 2 comments![]( "Open in Gerrit")
const std::string& auto_picture_in_picture_info) override;Consider using std::string_view instead of `const std::string&`
for (const auto& it : normal_players_) {Benjamin KeenIs there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
Tommy SteimelFor video conferencing it should be ok to skip surfacing this info. The conditions for AutoPiP are pretty straight forward (camera/mic permission). But I am open to include those cases as well.
Do you think we should use `ForAllPlayers` instead?
Benjamin KeenI guess what I'm saying is this doesn't seem to be a player-specific thing, so recording it for each player doesn't seem to make sense to me, though to be fair I don't know much about media log
Yes that's right. The log is not specific to a player, however media logs all seem to be assosicated to a player by design (DevTools -> Media), which is the reason I am surfacing the info for all players.
@tmath...@chromium.org: WDYT of this approach? Or is there an alternative to add this information to the Media DevTools section w/o using the players?
From my understanding, the situation is that when a few conditions are met (like the permissions you mentioned), all players on the page become either eligable or ineligable for being auto-pipped, and you update the media logs for each of these players? If this is the case, it seems fine to me. I do suggest that you don't bother with additional string formatting here, since all the events get a name as well as a message, so you can just set the name to "auto-pip state" and then the message to whatever AutoPipReasonToString() returns.
Related details
- Benjamin Keen
- Tommy Steimel
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 2 comments![]( "Open in Gerrit")
const std::string& auto_picture_in_picture_info) override;Consider using std::string_view instead of `const std::string&`
Done
for (const auto& it : normal_players_) {Benjamin KeenIs there a reason we do this for every player and also not do this when there isn't a normal player? I think in many video conferencing cases there won't even be a normal player
Tommy SteimelFor video conferencing it should be ok to skip surfacing this info. The conditions for AutoPiP are pretty straight forward (camera/mic permission). But I am open to include those cases as well.
Do you think we should use `ForAllPlayers` instead?
Benjamin KeenI guess what I'm saying is this doesn't seem to be a player-specific thing, so recording it for each player doesn't seem to make sense to me, though to be fair I don't know much about media log
Ted (Chromium) MeyerYes that's right. The log is not specific to a player, however media logs all seem to be assosicated to a player by design (DevTools -> Media), which is the reason I am surfacing the info for all players.
@tmath...@chromium.org: WDYT of this approach? Or is there an alternative to add this information to the Media DevTools section w/o using the players?
From my understanding, the situation is that when a few conditions are met (like the permissions you mentioned), all players on the page become either eligable or ineligable for being auto-pipped, and you update the media logs for each of these players? If this is the case, it seems fine to me. I do suggest that you don't bother with additional string formatting here, since all the events get a name as well as a message, so you can just set the name to "auto-pip state" and then the message to whatever AutoPipReasonToString() returns.
Tx Ted, that's correct, I had not looked at it from that perspective but that's essentially what ends up taking place indirectly.
Updated to apply to all players.
don't bother with additional string formatting here
Done. I may need additional formatting in a follow up, but it makes sense to move the formatting to the events info class.
Related details
- Tommy Steimel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Steimel (Gerrit)
Tommy Steimel voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Keen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ted (Chromium) Meyer (Gerrit)
Ted (Chromium) Meyer added 1 comment![]( "Open in Gerrit")
media::PictureInPictureEventsInfo::AutoPipReasonToString(New patch is calling the to string method again for each player, move out and pass std::string_view to callback?
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
media::PictureInPictureEventsInfo::AutoPipReasonToString(New patch is calling the to string method again for each player, move out and pass std::string_view to callback?
- 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 2 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
2 comments
lgtm for the files I own % type change.
const WTF::String& auto_picture_in_picture_info) = 0;This should be either std::string_view or WebString I think.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 2 comments![]( "Open in Gerrit")
lgtm for the files I own % type change.
Done
const WTF::String& auto_picture_in_picture_info) = 0;This should be either std::string_view or WebString I think.
IIUC from this [document](https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#variants), the mojom Blink generated code uses `WTF::String`. Please let me know if the guidance has changed.
Related details
- Dale Curtis
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
const WTF::String& auto_picture_in_picture_info) = 0;Benjamin KeenThis should be either std::string_view or WebString I think.
IIUC from this [document](https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#variants), the mojom Blink generated code uses `WTF::String`. Please let me know if the guidance has changed.
I don't think we're supposed to use WTF types in public/ interfaces:
WebString should be interchangeable.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 2 comments![]( "Open in Gerrit")
Adding:
* @bo...@chromium.org for:
* c/b/p/picture_in_picture_service_impl_unittest.cc
* @mfo...@chromium.org for:
* t/b/p/w/m/m/web_media_player_ms.h
* @har...@chromium.org for:
* t/b/p/p/web_media_player.h
* t/b/r/p/t/empty_web_media_player.h
* t/b/r/m/w/audio_context.h
Benjamin KeenThis should be either std::string_view or WebString I think.
Dale CurtisIIUC from this [document](https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/cpp/bindings/README.md#variants), the mojom Blink generated code uses `WTF::String`. Please let me know if the guidance has changed.
I don't think we're supposed to use WTF types in public/ interfaces:
WebString should be interchangeable.
- Bo Liu
- Kentaro Hara
- Mark Foltz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen voted Commit-Queue+1![]( "Open in Gerrit")
- Bo Liu
- Chromium IPC Reviews
- Kentaro Hara
- Mark Foltz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kentaro Hara
| Code-Review | +1 |
1 comment
- Benjamin Keen
- Bo Liu
- Chromium IPC Reviews
- Mark Foltz
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ke...@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): ke...@chromium.org
Reviewer source(s):
ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Benjamin Keen
- Bo Liu
- Ken Buchanan
- Mark Foltz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bo Liu
| Code-Review | +1 |
1 comment
- Benjamin Keen
- Ken Buchanan
- Mark Foltz
Ken Buchanan (Gerrit)
Ken Buchanan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ken Buchanan
| Code-Review | +1 |
1 comment
- Benjamin Keen
- Mark Foltz
Benjamin Keen (Gerrit)
Benjamin Keen voted Commit-Queue+2![]( "Open in Gerrit")
- Mark Foltz
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Surface within DevTools AutoPiP for media playback conditions
This information will help developers identify why a picture in picture
window was/was not opened automatically for media playback.
This CL only plumbs the AutoPiP reason, a follow up CL will add the
remaining conditions.
- M content/browser/media/session/media_session_controller.cc
- M content/browser/media/session/media_session_controller.h
- M content/browser/media/session/media_session_controller_unittest.cc
- M content/browser/media/session/media_session_impl.cc
- M content/browser/media/session/media_session_impl.h
- M content/browser/media/session/media_session_impl_service_routing_unittest.cc
- M content/browser/media/session/media_session_impl_unittest.cc
- M content/browser/media/session/media_session_player_observer.h
- M content/browser/media/session/media_session_service_impl_browsertest.cc
- M content/browser/media/session/mock_media_session_player_observer.cc
- M content/browser/media/session/mock_media_session_player_observer.h
- M content/browser/media/session/pepper_player_delegate.h
- M content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc
- M media/base/media_log_events.h
- M media/mojo/mojom/media_player.mojom
- M third_party/blink/public/platform/web_media_player.h
- M third_party/blink/public/web/modules/mediastream/web_media_player_ms.h
- M third_party/blink/renderer/core/html/media/html_media_element.cc
- M third_party/blink/renderer/core/html/media/html_media_element.h
- M third_party/blink/renderer/modules/mediacapturefromelement/html_video_element_capturer_source_unittest.cc
- M third_party/blink/renderer/modules/webaudio/audio_context.h
- M third_party/blink/renderer/platform/media/web_media_player_impl.cc
- M third_party/blink/renderer/platform/media/web_media_player_impl.h
- M third_party/blink/renderer/platform/testing/empty_web_media_player.h
Code-Review: +1 by Kentaro Hara, +1 by Bo Liu, +1 by Tommy Steimel, +1 by Ken Buchanan, +1 by Dale Curtis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |