[ImmersivePlayback] Integrate recommended spatial format to immersive playback session [chromium/src : main]
Gurmeet Kalra (Gerrit)
Gurmeet Kalra added 5 comments![]( "Open in Gerrit")
Lgtm for changes to the ./browser/media/immersive_playback/* component.
boolean isRecommended = mIsRecommended != null ? mIsRecommended : false;Can be combined as
```
activity.setImmersiveVideoOptions(stereoMode, projectionType, Boolean.TRUE.equals(mIsRecommended));
```
private void updateSpatialHeight() {Does the spatial height need to be updated every time the panel is shown? If the content of the RadioGroup doesn't change after initialization, this could be done only once.
var view = assumeNonNull(mView);
var mediator = assumeNonNull(mMediator);Also, is assumeNonNull check necessary here too?
var mediator = assumeNonNull(mMediator);Since ensureInitialized() is called at the start of show(), are these assumeNonNull checks still necessary for mMediator, mHolder, and mView?
Related details
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 5 comments![]( "Open in Gerrit")
@gurm...@google.com
I've refactored code a little moving the recommended options to the properties where they belong.
boolean isRecommended = mIsRecommended != null ? mIsRecommended : false;Can be combined as
```
activity.setImmersiveVideoOptions(stereoMode, projectionType, Boolean.TRUE.equals(mIsRecommended));
```
Done
Does the spatial height need to be updated every time the panel is shown? If the content of the RadioGroup doesn't change after initialization, this could be done only once.
Good point! Done.
var view = assumeNonNull(mView);
var mediator = assumeNonNull(mMediator);Also, is assumeNonNull check necessary here too?
mView/mMediator are nullable and we must do null checks.
Since ensureInitialized() is called at the start of show(), are these assumeNonNull checks still necessary for mMediator, mHolder, and mView?
I've discovered `EnsuresNonNull`/`RequiresNonNull` annotations that can be used to simplify this.
Related details
- Gurmeet Kalra
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gurmeet Kalra
| Code-Review | +1 |
1 comment
slgtm for changes to the ./browser/media/immersive_playback/* component.
Related details
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
@rak...@chromium.org
I'm looking for an owner's approval for `content/public/browser/immersive_playback_options.h`
Related details
- Frank Liberato
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
result.options->is_recommended =it seems a little out of place that default means 'kMono, kQuad' in the middle of the service.
would it fit better if `RequestImmersivePlaybackConfirmation` made the definitive call about defaultness, and set `is_recommended` properly rather than adjusting it here? i haven't looked through the java side yet, but i assume that there's already an analogous check for defaultness there.
or, alternatively, should the renderer set 'is recommended' in the options it sends to the service if it knows they're recommended? i don't know which one makes more sense.
or, maybe, should the spatial format be an optional in StartSession to indicate default? then any non-empty format is recommended.
the first option is likely self-contained in this CL. the second one is probably not, and the third one is almost certainly a follow-up if you want to go that direction.
so if you're aiming for branch, maybe option 1 is good for now and you can consider other alternatives post-branch.
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
- Rakina Zata Amni
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
it seems a little out of place that default means 'kMono, kQuad' in the middle of the service.
would it fit better if `RequestImmersivePlaybackConfirmation` made the definitive call about defaultness, and set `is_recommended` properly rather than adjusting it here? i haven't looked through the java side yet, but i assume that there's already an analogous check for defaultness there.
or, alternatively, should the renderer set 'is recommended' in the options it sends to the service if it knows they're recommended? i don't know which one makes more sense.
or, maybe, should the spatial format be an optional in StartSession to indicate default? then any non-empty format is recommended.
the first option is likely self-contained in this CL. the second one is probably not, and the third one is almost certainly a follow-up if you want to go that direction.
so if you're aiming for branch, maybe option 1 is good for now and you can consider other alternatives post-branch.
Moved this part to `TabWebContentsDelegateAndroid::RequestImmersivePlaybackConfirmation` for now. Will follow up with a deeper refactor later.
Related details
- Frank Liberato
- Frank Liberato
- Gurmeet Kalra
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted Code-Review+1![]( "Open in Gerrit")
- Frank Liberato
- Frank Liberato
- Oleh Desiatyrikov (xWF)
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted and added 1 comment![]( "Open in Gerrit")
Votes added by Frank Liberato
| Code-Review | +1 |
1 comment
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gurmeet Kalra
| Code-Review | +1 |
slgtm
Related details
- Frank Liberato
- Frank Liberato
- Oleh Desiatyrikov (xWF)
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sky Malice voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Frank Liberato
- Frank Liberato
- Oleh Desiatyrikov (xWF)
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Frank Liberato
- Frank Liberato
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Frank Liberato
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[ImmersivePlayback] Integrate recommended spatial format to immersive playback session
Dynamically populates and checks recommended formats in the radio group.
This change adds a new "Recommended" option to a in-session format
selection dropdown, so the user can switch back to a recommended
(default) format after they used an alternative options from the
dropdown.
Video used for testing: https://vimeo.com/1202570008
- M chrome/android/java/src/org/chromium/chrome/browser/media/VideoOverlayActivity.java
- M chrome/android/java/src/org/chromium/chrome/browser/media/immersive_playback/ImmersiveVideoFormatRadioGroup.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/ImmersiveVideoPlaybackActivity.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/ImmersiveVideoPlaybackCoordinator.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatCoordinator.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatMediator.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatProperties.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatSpatialView.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatView.java
- M chrome/android/modules/xr/internal/java/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatViewBinder.java
- M chrome/android/modules/xr/internal/javatests/src/org/chromium/chrome/browser/media/immersive_playback/ImmersiveVideoPlaybackActivityTest.java
- M chrome/android/modules/xr/internal/junit/src/org/chromium/chrome/browser/media/immersive_playback/ImmersiveVideoPlaybackCoordinatorTest.java
- M chrome/android/modules/xr/internal/junit/src/org/chromium/chrome/browser/media/immersive_playback/components/ImmersiveVideoFormatCoordinatorTest.java
- M chrome/android/modules/xr/internal/res/layout/immersive_video_format_view.xml
- M chrome/browser/android/tab_web_contents_delegate_android.cc
- M chrome/browser/ui/android/overlay/overlay_window_android.cc
- M chrome/browser/ui/android/strings/android_chrome_strings.grd
- M content/public/browser/immersive_playback_options.h
Code-Review: +1 by Sky Malice, +1 by Gurmeet Kalra, +1 by Rakina Zata Amni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |