Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS and COMMAND_ADJUST_DEVICE_VOLUME_WITH_FLAGS does not trigger related handlers in SimpleBasePlayer when using Media3 1.1.0 #554

Closed
ashutoshgngwr opened this issue Aug 1, 2023 · 8 comments
Assignees

Comments

@ashutoshgngwr
Copy link

With Media3 1.0.0, we could handle cast device volume by implementing COMMAND_GET_DEVICE_VOLUME, COMMAND_SET_DEVICE_VOLUME and COMMAND_ADJUST_DEVICE_VOLUME commands in SimpleBasePlayer. We used cast API to get/set device volume. For implementation, see this and this. Here, RemoteDeviceVolumeProvider is a simple abstraction that deals with the cast API. As a result, we could adjust the cast device volume using volume rockers during an active cast session.

With Media3 1.1.0, the latter two commands are now deprecated in favour of COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS and COMMAND_ADJUST_DEVICE_VOLUME_WITH_FLAGS. However, when we replaced the deprecated commands with the new ones in our existing implementation, the SimpleBasePlayer stopped invoking handleSetDeviceVolume and other related handlers. As a result, nothing happens on pressing volume rockers during a cast playback session.

But if we continue using the deprecated commands, everything works as expected.

Is it a bug or an intended change? More importantly, how do we continue to use volume rockers to adjust cast device volume when the cast session is active while incorporating the latest changes?

@oceanjules
Copy link
Contributor

Hi @ashutoshgngwr,

Could you confirm that you are now overriding the handler that has the following signature:

@ForOverride
protected ListenableFuture<?> handleSetDeviceVolume(
@IntRange(from = 0) int deviceVolume, int flags) {

You can see that this is the handler that is triggered in both the new and deprecated methods in lines 2725 and 2738:

@Override
public final void setDeviceVolume(int volume) {
verifyApplicationThreadAndInitState();
// Use a local copy to ensure the lambda below uses the current state value.
State state = this.state;
if (!shouldHandleCommand(Player.COMMAND_SET_DEVICE_VOLUME)) {
return;
}
updateStateForPendingOperation(
/* pendingOperation= */ handleSetDeviceVolume(volume, C.VOLUME_FLAG_SHOW_UI),
/* placeholderStateSupplier= */ () -> state.buildUpon().setDeviceVolume(volume).build());
}
@Override
public final void setDeviceVolume(int volume, @C.VolumeFlags int flags) {
verifyApplicationThreadAndInitState();
// Use a local copy to ensure the lambda below uses the current state value.
State state = this.state;
if (!shouldHandleCommand(Player.COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS)) {
return;
}
updateStateForPendingOperation(
/* pendingOperation= */ handleSetDeviceVolume(volume, flags),
/* placeholderStateSupplier= */ () -> state.buildUpon().setDeviceVolume(volume).build());
}

@oceanjules oceanjules self-assigned this Aug 1, 2023
@ashutoshgngwr
Copy link
Author

@oceanjules Hi there! Yes, I did implement the updated handlers (with flags). Moreover, the updated handlers appear to be working with deprecated commands but not with their replacements. Let me try to reproduce this in a minimal setting.

@ashutoshgngwr
Copy link
Author

@oceanjules Here's a minimal Android project demonstrating this issue. If you uncomment these lines, you see log entries for the handlers in question on running the app and pressing volume rockers on the device. But handlers don't get invoked on commenting out the deprecated commands.

@oceanjules
Copy link
Contributor

We also have a test that should check this behaviour (that also currently works) which you can use as a template for your minimal example:

@Test
public void setDeviceVolume_immediateHandling_updatesStateAndInformsListeners() {
State state =
new State.Builder()
.setAvailableCommands(new Commands.Builder().addAllCommands().build())
.build();
// Set a different one to the one requested to ensure the updated state is used.
State updatedState = state.buildUpon().setDeviceVolume(6).build();
AtomicInteger flagsFromHandlerRef = new AtomicInteger();
int volumeFlags = C.VOLUME_FLAG_SHOW_UI | C.VOLUME_FLAG_VIBRATE;
SimpleBasePlayer player =
new SimpleBasePlayer(Looper.myLooper()) {
private State playerState = state;
@Override
protected State getState() {
return playerState;
}
@Override
protected ListenableFuture<?> handleSetDeviceVolume(int volume, int flags) {
playerState = updatedState;
flagsFromHandlerRef.set(flags);
return Futures.immediateVoidFuture();
}
};
Listener listener = mock(Listener.class);
player.addListener(listener);
player.setDeviceVolume(3, volumeFlags);
assertThat(player.getDeviceVolume()).isEqualTo(6);
verify(listener).onDeviceVolumeChanged(6, /* muted= */ false);
assertThat(flagsFromHandlerRef.get()).isEqualTo(volumeFlags);
verifyNoMoreInteractions(listener);
}

@ashutoshgngwr
Copy link
Author

We also have a test that should check this behaviour (that also currently works)

Couldn't that imply that the problem might be somewhere else?

which you can use as a template for your minimal example

Why?

@oceanjules
Copy link
Contributor

Having tried your minimal example, I can only see this through logcat:

MediaSessionRecord      system_server     D  No routing session for com.example.myapplication
MediaSessionService     system_server     D  Adjusting suggestedStream=-2147483648 by 0. flags=4116, preferSuggestedStream=false, session=null

which comes from the MediaSessionRecord's "canHandleVolumeKey". I guess because isPlaybackTypeLocal() is false.

Setting the device info type to Local, gives us:

MediaSessionService     system_server D  dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=27424, uid=10316, asSystem=true, event=KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_VOLUME_DOWN, scanCode=114, metaState=0, flags=0x8, repeatCount=0, eventTime=434166456228000, downTime=434166456228000, deviceId=2, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     system_server D  Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by -1. flags=4113, suggestedStream=-2147483648, preferSuggestedStream=false

So I think all these key events are not going through Exoplayer at all, which is why I am not sure how you managed to get those log statement in the handlers working with the provided minimal setup.

@ashutoshgngwr
Copy link
Author

I am getting the following logs when running my example on an Android 11 (actual device) with the remote device type.

MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13269, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_VOLUME_DOWN, scanCode=114, metaState=0, flags=0x8, repeatCount=0, eventTime=43458071, downTime=43458071, deviceId=5, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by -1. flags=4113, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13269, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_UP, keyCode=KEYCODE_VOLUME_DOWN, scanCode=114, metaState=0, flags=0x8, repeatCount=0, eventTime=43458200, downTime=43458071, deviceId=5, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 0. flags=4116, suggestedStream=-2147483648, preferSuggestedStream=false

If I replace *_WITH_FLAGS commands with their deprecated counterparts, I am getting the following.

MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_VOLUME_UP, scanCode=115, metaState=0, flags=0x8, repeatCount=0, eventTime=43798183, downTime=43798183, deviceId=2, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 1. flags=4113, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionPlayer      handleIncreaseDeviceVolume: flags=1
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_UP, keyCode=KEYCODE_VOLUME_UP, scanCode=115, metaState=0, flags=0x8, repeatCount=0, eventTime=43798360, downTime=43798183, deviceId=2, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 0. flags=4116, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_VOLUME_DOWN, scanCode=114, metaState=0, flags=0x8, repeatCount=0, eventTime=43798951, downTime=43798951, deviceId=5, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by -1. flags=4113, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionPlayer      handleDecreaseDeviceVolume: flags=1
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_UP, keyCode=KEYCODE_VOLUME_DOWN, scanCode=114, metaState=0, flags=0x8, repeatCount=0, eventTime=43799132, downTime=43798951, deviceId=5, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 0. flags=4116, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_VOLUME_UP, scanCode=115, metaState=0, flags=0x8, repeatCount=0, eventTime=43799771, downTime=43799771, deviceId=2, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 1. flags=4113, suggestedStream=-2147483648, preferSuggestedStream=false
MediaSessionPlayer      handleIncreaseDeviceVolume: flags=1
MediaSessionService     dispatchVolumeKeyEvent, pkg=com.example.myapplication, opPkg=com.example.myapplication, pid=13986, uid=10377, asSystem=true, event=KeyEvent { action=ACTION_UP, keyCode=KEYCODE_VOLUME_UP, scanCode=115, metaState=0, flags=0x8, repeatCount=0, eventTime=43799950, downTime=43799771, deviceId=2, source=0x101, displayId=-1 }, stream=-2147483648, musicOnly=false
MediaSessionService     Adjusting com.example.myapplication/androidx.media3.session.id.Playback (userId=0) by 0. flags=4116, suggestedStream=-2147483648, preferSuggestedStream=false

I am not getting any entries with the MediaSessionRecord tag. Now I am not aware of how the internals work in the framework, but I think this should add some clarity.

@oceanjules
Copy link
Contributor

Hi @ashutoshgngwr,

Thank you for providing the Android version, that was the key piece of information that helped me reproduce it.

I have found the issue and we will be patching up a bugfix for it. For reference, these if-statements, should have had the new commands as well. Otherwise, the volumeControlType was set as FIXED, i.e. not changeable

Commands availableCommands = getAvailableCommands();
int volumeControlType = VolumeProviderCompat.VOLUME_CONTROL_FIXED;
if (availableCommands.contains(COMMAND_ADJUST_DEVICE_VOLUME)) {
volumeControlType = VolumeProviderCompat.VOLUME_CONTROL_RELATIVE;
if (availableCommands.contains(COMMAND_SET_DEVICE_VOLUME)) {
volumeControlType = VolumeProviderCompat.VOLUME_CONTROL_ABSOLUTE;

Also for reference, the reason it didn't work for me was because I was testing with Android 12 and 13 and it's a known issue that the hardware buttons and volume of a cast device are currently not working:

https://issuetracker.google.com/issues/201546605

tianyif pushed a commit that referenced this issue Aug 10, 2023
When hardware buttons are used to control the volume of the remote device, the call propagates to `MediaSessionCompat.setPlaybackToRemote(volumeProviderCompat)`. However, `volumeProviderCompat` was created incorrectly when the new device volume commands were present (COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS and COMMAND_ADJUST_DEVICE_VOLUME_WITH_FLAGS), i.e. with volumeControlType = `VOLUME_CONTROL_FIXED`. This resulted in `VolumeProviderCompat` which doesn't call `onSetVolumeTo` or `onAdjustVolume` and hence doesn't propagate the calls to the `Player`. Instead, it only worked with the deprecated commands which ensured the volumeControlType was `VOLUME_CONTROL_ABSOLUTE`.

This bug was introduced in c71e4bf (1.0 media 3 release) when `PlayerWrapper`'s call to `createVolumeProviderCompat` was mostly rewritten to handle the new commands, but the two if-statements were not amended. Note: this change fixes the bug only for Android 11 and below. For 12 and above, there is a tracking bug for the regression that was introduced: https://issuetracker.google.com/issues/201546605

http://Issue: #554

#minor-release

PiperOrigin-RevId: 554966361
tianyif pushed a commit that referenced this issue Aug 14, 2023
When hardware buttons are used to control the volume of the remote device, the call propagates to `MediaSessionCompat.setPlaybackToRemote(volumeProviderCompat)`. However, `volumeProviderCompat` was created incorrectly when the new device volume commands were present (COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS and COMMAND_ADJUST_DEVICE_VOLUME_WITH_FLAGS), i.e. with volumeControlType = `VOLUME_CONTROL_FIXED`. This resulted in `VolumeProviderCompat` which doesn't call `onSetVolumeTo` or `onAdjustVolume` and hence doesn't propagate the calls to the `Player`. Instead, it only worked with the deprecated commands which ensured the volumeControlType was `VOLUME_CONTROL_ABSOLUTE`.

This bug was introduced in c71e4bf (1.0 media 3 release) when `PlayerWrapper`'s call to `createVolumeProviderCompat` was mostly rewritten to handle the new commands, but the two if-statements were not amended. Note: this change fixes the bug only for Android 11 and below. For 12 and above, there is a tracking bug for the regression that was introduced: https://issuetracker.google.com/issues/201546605

http://Issue: #554

PiperOrigin-RevId: 554966361
(cherry picked from commit dedccc5)
microkatz pushed a commit to hugohlln/media that referenced this issue Sep 29, 2023
When hardware buttons are used to control the volume of the remote device, the call propagates to `MediaSessionCompat.setPlaybackToRemote(volumeProviderCompat)`. However, `volumeProviderCompat` was created incorrectly when the new device volume commands were present (COMMAND_SET_DEVICE_VOLUME_WITH_FLAGS and COMMAND_ADJUST_DEVICE_VOLUME_WITH_FLAGS), i.e. with volumeControlType = `VOLUME_CONTROL_FIXED`. This resulted in `VolumeProviderCompat` which doesn't call `onSetVolumeTo` or `onAdjustVolume` and hence doesn't propagate the calls to the `Player`. Instead, it only worked with the deprecated commands which ensured the volumeControlType was `VOLUME_CONTROL_ABSOLUTE`.

This bug was introduced in androidx@c71e4bf (1.0 media 3 release) when `PlayerWrapper`'s call to `createVolumeProviderCompat` was mostly rewritten to handle the new commands, but the two if-statements were not amended. Note: this change fixes the bug only for Android 11 and below. For 12 and above, there is a tracking bug for the regression that was introduced: https://issuetracker.google.com/issues/201546605

http://Issue: androidx#554

#minor-release

PiperOrigin-RevId: 554966361
@androidx androidx locked and limited conversation to collaborators Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants