DecodedAudioReady: Reconfigure sink on channel count change [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis added 5 comments![]( "Open in Gerrit")
// Do not override input parameters if we are allowing sink reconfiguration based onYou'd still only want to do this inside the IsValid() check below since this data comes from an untrusted process. You likely also only want to do this for `params.latency_tag() == AudioLatency::Type::kPlayback` otherwise this will effect things like WebRTC/WebAudio/etc.
The switch is fine for now, however does Android automotive have a custom build that can override the ContentRendererClient?
https://source.chromium.org/chromium/chromium/src/+/main:content/public/renderer/content_renderer_client.h;l=99;drc=10439bb6ee979d32a29e66937e2e5e2cb15d6bcf
If so we'd want to move this behavior there so it's embedder controlled instead of done via base::Feature flag (which is intended to be short lived).
MEDIA_EXPORT BASE_DECLARE_FEATURE(kSourceChannelCountChangedSinkReconfigure);How about `kMatchSourceAudioChannelLayout`?
sink_->Start();This needs to handle the case that `StopRendering_Locked()` occurs while device info is being requested.
DLOG(INFO) << __func__ << "() reconfiguring sink for channel count:" << buffer->channel_count();Possibly you need a new state:
`ChangeState_Locked(kReinitializingSink)`
That way you can handle the case where other calls occurs while the async portion of this method is going on.
}Note: Since you fall through here, the current buffer is likely converted to the original parameters and dropped on the floor before it gets a chance to play out.
It's probably better to just explicitly return here if that's the case (or reset the algorithm now and let it be appended into the new queue).
Related details
- Diego Valenzuela
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 6 comments![]( "Open in Gerrit")
Hi Dale,
I've addressed all your comments and pushed up my changes here:
https://chromium-review.googlesource.com/c/chromium/src/+/6811046
Unfortunately, after I added a Change-Id to the commit message, it resulted in a new Gerrit review so I will abandon this one and we can hop onto the new one.
Regards,
Michael
// Do not override input parameters if we are allowing sink reconfiguration based onYou'd still only want to do this inside the IsValid() check below since this data comes from an untrusted process. You likely also only want to do this for `params.latency_tag() == AudioLatency::Type::kPlayback` otherwise this will effect things like WebRTC/WebAudio/etc.
The switch is fine for now, however does Android automotive have a custom build that can override the ContentRendererClient?
https://source.chromium.org/chromium/chromium/src/+/main:content/public/renderer/content_renderer_client.h;l=99;drc=10439bb6ee979d32a29e66937e2e5e2cb15d6bcfIf so we'd want to move this behavior there so it's embedder controlled instead of done via base::Feature flag (which is intended to be short lived).
Acknowledged
MEDIA_EXPORT BASE_DECLARE_FEATURE(kSourceChannelCountChangedSinkReconfigure);Michael ChanHow about `kMatchSourceAudioChannelLayout`?
Acknowledged
sink_->Start();This needs to handle the case that `StopRendering_Locked()` occurs while device info is being requested.
Acknowledged
DLOG(INFO) << __func__ << "() reconfiguring sink for channel count:" << buffer->channel_count();Possibly you need a new state:
`ChangeState_Locked(kReinitializingSink)`That way you can handle the case where other calls occurs while the async portion of this method is going on.
Acknowledged
Note: Since you fall through here, the current buffer is likely converted to the original parameters and dropped on the floor before it gets a chance to play out.
It's probably better to just explicitly return here if that's the case (or reset the algorithm now and let it be appended into the new queue).
- Dale Curtis
- Diego Valenzuela
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 13 comments![]( "Open in Gerrit")
// For multichannel PCM playback, do not use power savingInteresting. Is it the case that we aren't opening true multi-channel streams without this flag even on normal Android devices?
For now at least, can we instead do something like `if !on_battery_power` or just always avoid power saving if ANDROID_AUTOMOTIVE? We wouldn't want this kicking in on normal devices until we understand it a bit more.
// Do not override input parameters if we allow reconfiguring sink based onI don't think we need this here, the client can just open the request with whatever params they want.
output_params = params;I think you only want to change the channel layout/count right? If you also want to use the source sample rate ARI needs some different changes.
"kMatchSourceAudioChannelLayout",No k prefix in the string name
(void)output_device_info; // Unused.Just put `/* output_device_info */` in the method declaration.
base::AutoLock auto_lock(lock_);Don't hold the lock while calling into the sink_ since it may be reentrant. You'll want to changeState first, then start the sink.
if (state_ == kPlaying || state_ == kReinitializingSink) {Shouldn't this be checking Uninit/Init/Reinit?
} else if (state_ == kPlaying || state_ == kReinitializingSink ) {Run `git cl format` to ensure formatting is correct. There shouldn't be a space after the conditional.
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Why change this? It seems like just adding `kReinitializingSink` here would be okay.
if (state_ != kPlaying && state_ != kReinitializingSink) {This shouldn't occur if you call sink->Stop() before calling ChangeState_Locked() when you receive the buffer with a new channel count. We can instead add a `DCHECK_NE(state_, kReinitializingSink);` at the top of this method.
} else if ((state_ == kPlaying || state_ == kReinitializingSink) &&Ditto.
(state_ == kPlaying || state_ == kReinitializingSink) &&Ditto.
case kReinitializingSink:I think this needs to go with kPlaying below since you could get this error during this time.
Related details
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maya Jelonkiewicz (Gerrit)
Maya Jelonkiewicz added 2 comments![]( "Open in Gerrit")
// For multichannel PCM playback, do not use power savingInteresting. Is it the case that we aren't opening true multi-channel streams without this flag even on normal Android devices?
For now at least, can we instead do something like `if !on_battery_power` or just always avoid power saving if ANDROID_AUTOMOTIVE? We wouldn't want this kicking in on normal devices until we understand it a bit more.
`AAUDIO_PERFORMANCE_MODE_POWER_SAVING` is loosely defined. Maybe it is possible for it alone to cause internal downmixing, though I'd find that unexpected. I'm curious what the automotive folks have observed here.
MEDIA_EXPORT BASE_DECLARE_FEATURE(kMatchSourceAudioChannelLayout);@dalec...@chromium.org Do we have a preference between adding features to `media_switches.*` vs `audio_features.*`?
Related details
- Diego Valenzuela
Dean Wheatley (Gerrit)
Dean Wheatley added 1 comment![]( "Open in Gerrit")
// For multichannel PCM playback, do not use power savingMaya JelonkiewiczInteresting. Is it the case that we aren't opening true multi-channel streams without this flag even on normal Android devices?
For now at least, can we instead do something like `if !on_battery_power` or just always avoid power saving if ANDROID_AUTOMOTIVE? We wouldn't want this kicking in on normal devices until we understand it a bit more.
`AAUDIO_PERFORMANCE_MODE_POWER_SAVING` is loosely defined. Maybe it is possible for it alone to cause internal downmixing, though I'd find that unexpected. I'm curious what the automotive folks have observed here.
See https://issuetracker.google.com/issues/395919023 for the problem (power saving mode && multi-channel direct output && Android Automotive dynamic audio policy are not mutually compatible) introduced in Android 15 (https://android.googlesource.com/platform/frameworks/av/+/806170e2a6532a42b34230fe7208cabee2aa5caa) and carried over to Android 16. Prior to Android 15, power saving (deep buffer) and direct output were permitted.
Many Android Automotive (and other) devices support multi-channel PCM output (without channel fidelity loss) to audio HAL via direct outputs only. To avoid the possible restriction in these AOSP audio framework versions, we avoid the power saving mode.
Not sure how many of these quirks you want to bake in.
Michael Chan (Gerrit)
Michael Chan added 13 comments![]( "Open in Gerrit")
// For multichannel PCM playback, do not use power savingInteresting. Is it the case that we aren't opening true multi-channel streams without this flag even on normal Android devices?
For now at least, can we instead do something like `if !on_battery_power` or just always avoid power saving if ANDROID_AUTOMOTIVE? We wouldn't want this kicking in on normal devices until we understand it a bit more.
Yes, on an AAOS15 emulator, I observe that a stereo non-direct mixer output is opened instead of a direct multichannel output.
I'll add in a guard on whether we are running on an automotive device to avoid power saving mode for now.
// Do not override input parameters if we allow reconfiguring sink based onI don't think we need this here, the client can just open the request with whatever params they want.
Acknowledged
output_params = params;I think you only want to change the channel layout/count right? If you also want to use the source sample rate ARI needs some different changes.
Yes, I only want to change the channel layout/count here. I'll update accordingly.
"kMatchSourceAudioChannelLayout",No k prefix in the string name
Acknowledged
(void)output_device_info; // Unused.Just put `/* output_device_info */` in the method declaration.
Acknowledged
base::AutoLock auto_lock(lock_);Don't hold the lock while calling into the sink_ since it may be reentrant. You'll want to changeState first, then start the sink.
Acknowledged
if (state_ == kPlaying || state_ == kReinitializingSink) {Shouldn't this be checking Uninit/Init/Reinit?
This looks incorrect, I will revert.
} else if (state_ == kPlaying || state_ == kReinitializingSink ) {Run `git cl format` to ensure formatting is correct. There shouldn't be a space after the conditional.
Acknowledged
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Why change this? It seems like just adding `kReinitializingSink` here would be okay.
I am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
if (state_ != kPlaying && state_ != kReinitializingSink) {This shouldn't occur if you call sink->Stop() before calling ChangeState_Locked() when you receive the buffer with a new channel count. We can instead add a `DCHECK_NE(state_, kReinitializingSink);` at the top of this method.
Acknowledged
} else if ((state_ == kPlaying || state_ == kReinitializingSink) &&Michael ChanDitto.
Acknowledged
(state_ == kPlaying || state_ == kReinitializingSink) &&Michael ChanDitto.
Acknowledged
case kReinitializingSink:I think this needs to go with kPlaying below since you could get this error during this time.
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
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 8 comments![]( "Open in Gerrit")
if (params_.channels() > 2 && manager->IsAutomotive()) {No need to get this from AudioManager, you can get it from `base::android::BuildInfo::GetInstance()->is_automotive()`
Michael ChanI think you only want to change the channel layout/count right? If you also want to use the source sample rate ARI needs some different changes.
Yes, I only want to change the channel layout/count here. I'll update accordingly.
Done
{This part should be happening automatically as buffers come in I think.
if (state_ == kPlaying || state_ == kReinitializingSink) {Michael ChanShouldn't this be checking Uninit/Init/Reinit?
This looks incorrect, I will revert.
Done
sink_->Stop();Don't call into `sink_` while holding `lock_`. Call this before ChangeState
// new channel count and layout parameters.Add a note here that this drops all in progress audio and will cause glitches, but is deemed acceptable for this use case.
// Asynchronously reconfigure the sink with the new parameters.Again, don't call into `sink_` while holding the lock.
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
I am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
I don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 6 comments![]( "Open in Gerrit")
if (params_.channels() > 2 && manager->IsAutomotive()) {No need to get this from AudioManager, you can get it from `base::android::BuildInfo::GetInstance()->is_automotive()`
Acknowledged
This part should be happening automatically as buffers come in I think.
Without this, what I observe is that after my initial playback completes to the end and I wait approximately 60 seconds before pressing play again, the progress bar stays at 0:00 after pressing play and playback doesn't proceed.
sink_->Stop();Don't call into `sink_` while holding `lock_`. Call this before ChangeState
Acknowledged
// new channel count and layout parameters.Add a note here that this drops all in progress audio and will cause glitches, but is deemed acceptable for this use case.
Acknowledged
// Asynchronously reconfigure the sink with the new parameters.Again, don't call into `sink_` while holding the lock.
Acknowledged
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
I don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
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")
Michael ChanThis part should be happening automatically as buffers come in I think.
Without this, what I observe is that after my initial playback completes to the end and I wait approximately 60 seconds before pressing play again, the progress bar stays at 0:00 after pressing play and playback doesn't proceed.
Hmm, is end of stream not rendering correctly? You might need to reset `first_packet_timestamp_` to point to the first packet played on the new sink. I.e., the one given at the time you call Stop() and reinitialize the sink.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
Michael ChanThis part should be happening automatically as buffers come in I think.
Dale CurtisWithout this, what I observe is that after my initial playback completes to the end and I wait approximately 60 seconds before pressing play again, the progress bar stays at 0:00 after pressing play and playback doesn't proceed.
Hmm, is end of stream not rendering correctly? You might need to reset `first_packet_timestamp_` to point to the first packet played on the new sink. I.e., the one given at the time you call Stop() and reinitialize the sink.
I tried your suggestion but the behavior is the same.
Below shows the last couple of logs from AttemptResumePlayback_Locked(), up to the end of playback:
08-07 08:08:46.092 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.136 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.224 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=1 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
However, when I attempt to play again after waiting 60 seconds, I do not get any more of these logs.
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
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")
Michael ChanThis part should be happening automatically as buffers come in I think.
Dale CurtisWithout this, what I observe is that after my initial playback completes to the end and I wait approximately 60 seconds before pressing play again, the progress bar stays at 0:00 after pressing play and playback doesn't proceed.
Michael ChanHmm, is end of stream not rendering correctly? You might need to reset `first_packet_timestamp_` to point to the first packet played on the new sink. I.e., the one given at the time you call Stop() and reinitialize the sink.
I tried your suggestion but the behavior is the same.
Below shows the last couple of logs from AttemptResumePlayback_Locked(), up to the end of playback:
08-07 08:08:46.092 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.136 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.224 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=1 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
However, when I attempt to play again after waiting 60 seconds, I do not get any more of these logs.
Ended should be rendered here https://source.chromium.org/chromium/chromium/src/+/main:media/renderers/audio_renderer_impl.cc;l=1407;drc=4af27b5b2abb0db430e4d326c26a74a6c34dcfa8 so I'd guess that's not happening for some reason.
Try also resetting the audio clock after stopping the sinkl.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
Michael ChanThis part should be happening automatically as buffers come in I think.
Dale CurtisWithout this, what I observe is that after my initial playback completes to the end and I wait approximately 60 seconds before pressing play again, the progress bar stays at 0:00 after pressing play and playback doesn't proceed.
Michael ChanHmm, is end of stream not rendering correctly? You might need to reset `first_packet_timestamp_` to point to the first packet played on the new sink. I.e., the one given at the time you call Stop() and reinitialize the sink.
Dale CurtisI tried your suggestion but the behavior is the same.
Below shows the last couple of logs from AttemptResumePlayback_Locked(), up to the end of playback:
08-07 08:08:46.092 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.136 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
08-07 08:08:46.181 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=0 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=1
08-07 08:08:46.224 3387 3475 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:417] AttemptResumePlayback_Locked mzchan: buffering_state=1 received_end_of_stream=1 should_render_end_of_stream=0 algorithm_->IsQueueAdequateForPlayback()=0
However, when I attempt to play again after waiting 60 seconds, I do not get any more of these logs.
Ended should be rendered here https://source.chromium.org/chromium/chromium/src/+/main:media/renderers/audio_renderer_impl.cc;l=1407;drc=4af27b5b2abb0db430e4d326c26a74a6c34dcfa8 so I'd guess that's not happening for some reason.
Try also resetting the audio clock after stopping the sinkl.
I just tried out resetting the audio clock. In any case, it looks like that does happen and we are invoking OnPlaybackEnded(). Still the problem persists:
08-07 09:57:18.446 11816 11947 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:1489] Render() mzchan: audio_clock_->front_timestamp(): 31.9192 s, ended_timestamp_: 31.968 s, rendered_end_of_stream_: 0
08-07 09:57:18.490 11816 11947 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:1489] Render() mzchan: audio_clock_->front_timestamp(): 31.9632 s, ended_timestamp_: 31.968 s, rendered_end_of_stream_: 0
08-07 09:57:18.534 11816 11947 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:1489] Render() mzchan: audio_clock_->front_timestamp(): 31.9709 s, ended_timestamp_: 31.968 s, rendered_end_of_stream_: 0
08-07 09:57:18.534 11816 11947 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:1497] Render() mzchan: calling OnPlaybackEnded() with front_timestamp: 31.9709 s, ended_timestamp_: 31.968 s
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
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")
Hmmm, is something setting BUFFERING_HAVE_NOTHING? The code you have uploaded resets to BUFFERING_HAVE_ENOUGH, which seems to be the only difference with your approach if OnPlaybackEnded is being called.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
What I am observing is that AudioRendererImpl itself is getting destructed after approximately 15 seconds of inactivity. On attempting playback, a new AudioRenderImpl is created and therefore initializes buffering_state_ to BUFFERING_HAVE_NOTHING:
08-11 09:46:37.235 32742 32757 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:82] AudioRendererImpl() mzchan: Constructed
08-11 09:46:37.307 32742 32766 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:458] OnSourceChannelCountChanged() mzchan: received_end_of_stream_=0 algorithm_->IsQueueAdequateForPlayback()=1 buffering_state_=0 first_packet_timestamp_ == kNoTimestamp:0 sink_playing_=0
08-11 09:46:37.307 32742 32766 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:466] OnSourceChannelCountChanged() mzchan: Resuming playback, setting buffering state to BUFFERING_HAVE_ENOUGH
08-11 09:46:37.307 32742 32766 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1604] SetBufferingState_Locked : 0 -> 1
08-11 09:46:52.218 32742 32766 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:118] ~AudioRendererImpl() mzchan: Destructed
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
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")
Hmm, that's strange. It sounds like the first playback is completing okay, but when you restart with the old params the new audio renderer isn't working.
So PlaybackEnded() is being called, we're setting HAVE_ENOUGH for ended correctly, and the pipeline is correctly suspending after inactivity. The sink is reused, but seems to be in a bad state.
Destruction of the renderer should stop the sink though: https://source.chromium.org/chromium/chromium/src/+/main:media/renderers/audio_renderer_impl.cc;l=108;drc=4af27b5b2abb0db430e4d326c26a74a6c34dcfa8
When the new renderer is created, does it complete GetOutputDeviceInfoAsync() correctly? Is it just that no new Render() calls come in when the new sink starts? Can you provide the full chrome://media-internals log? If you can't use chrome://media-internals dev tools media panel should have the log as well.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
- Ying Zheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
To answer your previous questions:
- GetOutputDeviceInfoAsync() completed correctly as I see both OnDeviceInfoReceived() and OnSourceChannelCountChanged() logs.
- No Render() calls were observed when the new sink starts.
- I'll email you the media-internals.txt file.
Here's some additional observations:
- Sometimes, I observe ~AudioRendererImpl() getting called 4 minutes after the AudioRendererImpl() constructor is invoked.
- Playback not starting issue occurs even on first attempt to play back, and also before ~AudioRendererImpl() gets called. Waiting ~30 seconds is sufficient. Please see the log below.
- In this case, I'm not seeing any DecodedAudioReady() nor Render() calls coming through on pressing play after waiting ~30 seconds after the AudioRendererImpl() constructor is invoked.
08-13 10:26:05.063 2990 3011 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:81] AudioRendererImpl() mzchan: constructor state_:0 sink_:0x77f573745a58
08-13 10:26:05.990 2990 3317 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:393] Initialize() mzchan: sink_:0x77f573745a58 null_sink_:0 state_:0
08-13 10:26:06.007 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:462] OnDeviceInfoReceived
08-13 10:26:06.007 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:495] OnDeviceInfoReceived: format: PCM_LOW_LATENCY, channel_layout: 3, channels: 2, sample_rate: 48000, frames_per_buffer: 2116, effects: NONE, mic_positions: , hw_capabilities: min_frames_per_buffer: 0, max_frames_per_buffer: 0, bitstream_formats:0, require_encapsulation:0, require_audio_offload:0
08-13 10:26:06.007 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:675] OnDeviceInfoReceived: is_passthrough_=0 codec=eac3 stream->audio_decoder_config().sample_format=2
08-13 10:26:06.071 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.071 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1013] DecodedAudioReady Reconfiguring sink for channel count from 6 to 8
08-13 10:26:06.071 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.074 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.078 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.078 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.082 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.089 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:942] DecodedAudioReady(0)
08-13 10:26:06.105 2990 3317 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:446] OnSourceChannelCountChanged() Reconfiguring sink for channel count:8
<Play pressed approximately 30 seconds after above log; No further DecodedAudioReady() logs>
08-13 10:30:05.883 2990 3317 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:106] ~AudioRendererImpl() mzchan: destructor sink_:0x77f573745a58 null_sink_:0x77f573747f10 state_:5
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 2 comments![]( "Open in Gerrit")
The ~AudioRendererImpl should just be the pipeline getting suspended on idle timeout or other behaviors (backgrounding a paused player, frame frozen, etc) that are normal, but could also happen if an error is occurring. All this should be in the media-internals log though (suspended, closed for error, etc).
If no Render() calls are coming in, it's not unexpected to see no more DecodedAudioReady() calls, since we're not exhausting the buffer and it probably filled up.
My next guess would be RendererImpl has called StopTicking() due to receiving a HAVE_NOTHING at some point (possibly from the VideoRendererImpl as time skips). Can you add logging to RendererImpl to see when buffering states are changing and when it's calling StopTicking/StartTicking().
Possibly your version of the patch is the one we want with one small modification: deliver a HAVE_NOTHING to the renderer while we go through the output device change (this may be happening automatically and explain a lot of this behavior). This would stop the video renderer and prevent any confusion on timings.
Can you e-mail a link to the sample stream which changes channel counts like this? I can test it locally early next week if we can't figure this out via the review.
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
Michael ChanI don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Please see above comment about issue starting playback.
To be clear, the logs you've provided via e-mail are from adding `case kReinitializingSink` to `case kPlaying`? Was that a playback all the way to the end w/ the playback not restarting error you described? I don't see an ended status in the log (or even a HAVE_NOTHING) which is surprising in that case if so.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
Michael ChanI don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Dale CurtisPlease see above comment about issue starting playback.
To be clear, the logs you've provided via e-mail are from adding `case kReinitializingSink` to `case kPlaying`? Was that a playback all the way to the end w/ the playback not restarting error you described? I don't see an ended status in the log (or even a HAVE_NOTHING) which is surprising in that case if so.
The media-internals.txt file I've provided is based on the code in this CL with the exception of commenting out the AttemptResumePlayback_Locked() call in OnSourceChannelCountChanged().
I've resent the media-internals.txt file via email with the initial observation of playback to the end and attempting playback again afterwards.
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
Michael ChanI don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Dale CurtisPlease see above comment about issue starting playback.
Michael ChanTo be clear, the logs you've provided via e-mail are from adding `case kReinitializingSink` to `case kPlaying`? Was that a playback all the way to the end w/ the playback not restarting error you described? I don't see an ended status in the log (or even a HAVE_NOTHING) which is surprising in that case if so.
The media-internals.txt file I've provided is based on the code in this CL with the exception of commenting out the AttemptResumePlayback_Locked() call in OnSourceChannelCountChanged().
I've resent the media-internals.txt file via email with the initial observation of playback to the end and attempting playback again afterwards.
Thanks, replied via e-mail.
If you comment out that call in OnSourceChannelCountChanged() you do need to merge the kPlaying and kReinitializeSink states in this block. Otherwise we're never handling ended correctly.
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Recitation-Check
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
Please see logs which I obtained when playing back for the first time to the end, then attempting to play again afterwards but encountering the issue:
08-14 19:07:02.181 8794 8815 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:81] AudioRendererImpl() mzchan: constructor state_:0 sink_:0x7cb8ecb930d8
08-14 19:07:02.187 8794 8870 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:393] Initialize() mzchan: sink_:0x7cb8ecb930d8 null_sink_:0 state_:0
08-14 19:07:02.230 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:461] OnDeviceInfoReceived
08-14 19:07:02.230 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:494] OnDeviceInfoReceived: format: PCM_LOW_LATENCY, channel_layout: 3, channels: 2, sample_rate: 48000, frames_per_buffer: 2116, effects: NONE, mic_positions: , hw_capabilities: min_frames_per_buffer: 0, max_frames_per_buffer: 0, bitstream_formats:0, require_encapsulation:0, require_audio_offload:0
08-14 19:07:02.230 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:674] OnDeviceInfoReceived: is_passthrough_=0 codec=eac3 stream->audio_decoder_config().sample_format=2
08-14 19:07:02.941 8794 8870 I chromium: [INFO:media/renderers/video_renderer_impl.cc:372] OnBufferingStateChange() mzchan: Buffering state changed to BUFFERING_HAVE_ENOUGH
08-14 19:07:02.941 8794 8870 I chromium: [INFO:media/renderers/renderer_impl.cc:54] OnBufferingStateChange() mzchan: Buffering state changed to 1 reason: 0
08-14 19:07:02.941 8794 8870 V chromium: [VERBOSE1:media/renderers/renderer_impl.cc:731] OnBufferingStateChange mzchan: video BUFFERING_HAVE_NOTHING -> BUFFERING_HAVE_ENOUGH
08-14 19:07:02.968 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1587] SetBufferingState_Locked : 0 -> 1
08-14 19:07:02.968 8794 8870 I chromium: [INFO:media/renderers/renderer_impl.cc:54] OnBufferingStateChange() mzchan: Buffering state changed to 1 reason: 0
08-14 19:07:02.968 8794 8870 V chromium: [VERBOSE1:media/renderers/renderer_impl.cc:731] OnBufferingStateChange mzchan: audio BUFFERING_HAVE_NOTHING -> BUFFERING_HAVE_ENOUGH
08-14 19:07:02.968 8794 8870 I chromium: [INFO:media/renderers/renderer_impl.cc:874] StartPlayback() mzchan: Start time source ticking
08-14 19:07:39.395 8794 8870 I chromium: [INFO:media/renderers/renderer_impl.cc:856] PausePlayback() mzchan: Stopping time source ticking
08-14 19:07:54.399 8794 8870 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:106] ~AudioRendererImpl() mzchan: destructor sink_:0x7cb8ecb930d8 null_sink_:0x7cb8ecbad070 state_:5
<Attempt to play again after first playback has ended>
08-14 19:07:58.824 8794 8815 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:81] AudioRendererImpl() mzchan: constructor state_:0 sink_:0x7cb8ecb930d8
08-14 19:07:58.826 8794 8870 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:393] Initialize() mzchan: sink_:0x7cb8ecb930d8 null_sink_:0 state_:0
08-14 19:07:58.831 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:461] OnDeviceInfoReceived
08-14 19:07:58.831 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:494] OnDeviceInfoReceived: format: PCM_LOW_LATENCY, channel_layout: 3, channels: 2, sample_rate: 48000, frames_per_buffer: 2116, effects: NONE, mic_positions: , hw_capabilities: min_frames_per_buffer: 0, max_frames_per_buffer: 0, bitstream_formats:0, require_encapsulation:0, require_audio_offload:0
08-14 19:07:58.831 8794 8870 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:674] OnDeviceInfoReceived: is_passthrough_=0 codec=eac3 stream->audio_decoder_config().sample_format=2
08-14 19:07:58.859 8794 8870 I chromium: [INFO:media/renderers/video_renderer_impl.cc:372] OnBufferingStateChange() mzchan: Buffering state changed to BUFFERING_HAVE_ENOUGH
08-14 19:07:58.859 8794 8870 I chromium: [INFO:media/renderers/renderer_impl.cc:54] OnBufferingStateChange() mzchan: Buffering state changed to 1 reason: 0
08-14 19:07:58.859 8794 8870 V chromium: [VERBOSE1:media/renderers/renderer_impl.cc:731] OnBufferingStateChange mzchan: video BUFFERING_HAVE_NOTHING -> BUFFERING_HAVE_ENOUGH
Note that the above was reproduced with this CL with the only modification of commenting out AttemptResumePlayback_Locked() in OnSourceChannelCountChanged(). I did try to merge "case kReinitializingSink" with "case kPlaying", but I got the same assert failure I saw earlier (see below), which got fixed after introducing the kReinitializingSink state:
08-14 13:11:38.753 3374 3432 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
The asset that I am using is available here, however I am relying on an integrated Dolby Digital Plus MediaCodec decoder to enable playback:
I wonder if concatenating AAC streams with different channel counts could trigger this behavior but would need some experimentation.
Related details
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 10 comments![]( "Open in Gerrit")
#endif // BUILDFLAG(IS_ANDROID)Don't guard this feature behind IS_ANDROID or you need to guard the whole thing. Prefer to just let this build on all platforms.
MEDIA_EXPORT BASE_DECLARE_FEATURE(kMatchSourceAudioChannelLayout);@dalec...@chromium.org Do we have a preference between adding features to `media_switches.*` vs `audio_features.*`?
I think this one has to go in media_switches since Media/renderers needs access to it.
bool AudioRendererImpl::AttemptResumePlayback_Locked(Revert this function change.
}This needs `else { sink_->Play(); }` to resolve the hung sink issues. The rest of AttemptResumePlayback_Locked() can be folded into the switch statement down below and things seem to work fine.
See the play() addition below, it should fix all these issues. When the AudioRendererMixerInput powering the sink is reused it doesn't auto-start. The new code doesn't handle that case.
bool allow_config_changes = false;Please revert this hunk.
// If the incoming buffer's channel count has changed,Use a pair of base::AutoUnlock within this function when calling into the sink instead of changing the locks elsewhere in this function.
sink_->Stop();This also needs to cleanup the `null_sink_` and make sure it's behaving correctly. I.e., stays on the nullsink as the channel count changes when muted, etc.
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
Michael ChanI don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Dale CurtisPlease see above comment about issue starting playback.
Michael ChanTo be clear, the logs you've provided via e-mail are from adding `case kReinitializingSink` to `case kPlaying`? Was that a playback all the way to the end w/ the playback not restarting error you described? I don't see an ended status in the log (or even a HAVE_NOTHING) which is surprising in that case if so.
Dale CurtisThe media-internals.txt file I've provided is based on the code in this CL with the exception of commenting out the AttemptResumePlayback_Locked() call in OnSourceChannelCountChanged().
I've resent the media-internals.txt file via email with the initial observation of playback to the end and attempting playback again afterwards.
Thanks, replied via e-mail.
If you comment out that call in OnSourceChannelCountChanged() you do need to merge the kPlaying and kReinitializeSink states in this block. Otherwise we're never handling ended correctly.
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Michael Chan (Gerrit)
Michael Chan added 10 comments![]( "Open in Gerrit")
Hi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Don't guard this feature behind IS_ANDROID or you need to guard the whole thing. Prefer to just let this build on all platforms.
Done
MEDIA_EXPORT BASE_DECLARE_FEATURE(kMatchSourceAudioChannelLayout);Dale Curtis@dalec...@chromium.org Do we have a preference between adding features to `media_switches.*` vs `audio_features.*`?
I think this one has to go in media_switches since Media/renderers needs access to it.
Acknowledged
bool AudioRendererImpl::AttemptResumePlayback_Locked(Michael ChanRevert this function change.
Done
This needs `else { sink_->Play(); }` to resolve the hung sink issues. The rest of AttemptResumePlayback_Locked() can be folded into the switch statement down below and things seem to work fine.
Done
Acknowledged
bool allow_config_changes = false;Michael ChanPlease revert this hunk.
Done
Use a pair of base::AutoUnlock within this function when calling into the sink instead of changing the locks elsewhere in this function.
Done
return !AttemptResumePlayback_Locked(should_render_end_of_stream);Michael ChanWhy change this? It seems like just adding `kReinitializingSink` here would be okay.
Dale CurtisI am reusing these lines in OnSourceChannelCountChanged() so I refactored this code into the new function AttemptResumePlayback_Locked() to avoid duplicating it. I'm okay to revert this here to minimize changes.
Michael ChanI don't see why the code needs to be duplicated? Isn't it enough to just add kReinitializingSink to the kPlaying case?
Dale CurtisPlease see above comment about issue starting playback.
Michael ChanTo be clear, the logs you've provided via e-mail are from adding `case kReinitializingSink` to `case kPlaying`? Was that a playback all the way to the end w/ the playback not restarting error you described? I don't see an ended status in the log (or even a HAVE_NOTHING) which is surprising in that case if so.
Dale CurtisThe media-internals.txt file I've provided is based on the code in this CL with the exception of commenting out the AttemptResumePlayback_Locked() call in OnSourceChannelCountChanged().
I've resent the media-internals.txt file via email with the initial observation of playback to the end and attempting playback again afterwards.
Dale CurtisThanks, replied via e-mail.
If you comment out that call in OnSourceChannelCountChanged() you do need to merge the kPlaying and kReinitializeSink states in this block. Otherwise we're never handling ended correctly.
As noted this section can be reverted.
- Dale Curtis
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 2 comments![]( "Open in Gerrit")
Hi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Hmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
// Attempt to resume playback depending on buffering and playback state.Delete this function
Related details
- Dean Wheatley
- Diego Valenzuela
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(state_, kPlaying);I am able to trigger this DCHECK by seeking while the reinitialize is in flight, so you do need to figure out how to handle a Flush() occurring during state kReinitializingSink. I suspect you just need to run the flush like normal.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 3 comments![]( "Open in Gerrit")
Dale CurtisHi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Hmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
I am unable to start playback of that asset - it shows a spinning play icon and pressing play doesn't start playback and the icon continues to spin.
With the Dolby Digital Plus asset, I can play through the first time but again, waiting like ~60 seconds before attempting to play again will cause the DCHECK failure above on AAOS15 emulator.
// Attempt to resume playback depending on buffering and playback state.Michael ChanDelete this function
Acknowledged
DCHECK_EQ(state_, kPlaying);I am able to trigger this DCHECK by seeking while the reinitialize is in flight, so you do need to figure out how to handle a Flush() occurring during state kReinitializingSink. I suspect you just need to run the flush like normal.
Are you suggesting we just allow state_ == kReinitializingSink here as well and continue flushing?
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 2 comments![]( "Open in Gerrit")
Dale CurtisHi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Michael ChanHmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
I am unable to start playback of that asset - it shows a spinning play icon and pressing play doesn't start playback and the icon continues to spin.
With the Dolby Digital Plus asset, I can play through the first time but again, waiting like ~60 seconds before attempting to play again will cause the DCHECK failure above on AAOS15 emulator.
Hmm, the asset is just AAC. Are you building with proprietary_codecs=true and ffmpeg_branding=Chrome?
DCHECK_EQ(state_, kPlaying);Michael ChanI am able to trigger this DCHECK by seeking while the reinitialize is in flight, so you do need to figure out how to handle a Flush() occurring during state kReinitializingSink. I suspect you just need to run the flush like normal.
Are you suggesting we just allow state_ == kReinitializingSink here as well and continue flushing?
Maybe, it's a bit complicated by the fact that Flush() will change state to kFlushing, so you may need to give it some more thought.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
Dale CurtisHi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Michael ChanHmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
Dale CurtisI am unable to start playback of that asset - it shows a spinning play icon and pressing play doesn't start playback and the icon continues to spin.
With the Dolby Digital Plus asset, I can play through the first time but again, waiting like ~60 seconds before attempting to play again will cause the DCHECK failure above on AAOS15 emulator.
Hmm, the asset is just AAC. Are you building with proprietary_codecs=true and ffmpeg_branding=Chrome?
Yes, I have those flags in my args.gn:
target_os = "android"
target_cpu = "x64" # See "Figuring out target_cpu" below
android_static_analysis = "build_server"
proprietary_codecs=true
enable_platform_ac3_eac3_audio=true
ffmpeg_branding="Chrome"
is_debug=true
dcheck_always_on = true
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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")
Dale CurtisHi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Michael ChanHmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
Dale CurtisI am unable to start playback of that asset - it shows a spinning play icon and pressing play doesn't start playback and the icon continues to spin.
With the Dolby Digital Plus asset, I can play through the first time but again, waiting like ~60 seconds before attempting to play again will cause the DCHECK failure above on AAOS15 emulator.
Michael ChanHmm, the asset is just AAC. Are you building with proprietary_codecs=true and ffmpeg_branding=Chrome?
Yes, I have those flags in my args.gn:
target_os = "android"
target_cpu = "x64" # See "Figuring out target_cpu" below
android_static_analysis = "build_server"
proprietary_codecs=true
enable_platform_ac3_eac3_audio=true
ffmpeg_branding="Chrome"
is_debug=true
dcheck_always_on = true
That all looks right. I'm not sure why the linked stream wouldn't work for you then. I'll try to poke at this more tomorrow when I have more time. That sink state does mean that there's some missing state case not being handled.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 4 comments![]( "Open in Gerrit")
Hi Dale,
I've added a unit test: AudioRendererImplTest.SinkReconfiguredOnChannelCountChange
Thanks,
Michael
// For multichannel PCM playback, do not use power savingMichael ChanInteresting. Is it the case that we aren't opening true multi-channel streams without this flag even on normal Android devices?
For now at least, can we instead do something like `if !on_battery_power` or just always avoid power saving if ANDROID_AUTOMOTIVE? We wouldn't want this kicking in on normal devices until we understand it a bit more.
Yes, on an AAOS15 emulator, I observe that a stereo non-direct mixer output is opened instead of a direct multichannel output.
I'll add in a guard on whether we are running on an automotive device to avoid power saving mode for now.
Done
Michael ChanI am able to trigger this DCHECK by seeking while the reinitialize is in flight, so you do need to figure out how to handle a Flush() occurring during state kReinitializingSink. I suspect you just need to run the flush like normal.
Dale CurtisAre you suggesting we just allow state_ == kReinitializingSink here as well and continue flushing?
Maybe, it's a bit complicated by the fact that Flush() will change state to kFlushing, so you may need to give it some more thought.
Done
sink_->Stop();Michael ChanThis also needs to cleanup the `null_sink_` and make sure it's behaving correctly. I.e., stays on the nullsink as the channel count changes when muted, etc.
Acknowledged
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 15 comments![]( "Open in Gerrit")
The state machine for ARI is already pretty complicated, we want to avoid adding more complexity for this side-feature.
}Lets make this an else with the clause above.
// Use input source audio channel count/layout if allowingDo we really need to keep just the channel count the same? I.e., if we prefer the stream sample rate, is that okay? I think it would let us drop this clause here and instead just add a new conditional to:
bool sink_in_playing_state_;Ditto, this should be removed.
// True if |sink_| was undergoing reinitialization prior to playing.I don't think we need this new state. The point of having a `state_` variable is to avoid the need for additional bool multipliers on the state diagram.
if ((state_ == kPlaying && was_reinitializing_sink_) ||Just early return with state == kReinitalizingSink.
Likewise early return if state == reinitializing after setting sink_playing_ = false.
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||This should only be state_ == kFlushed, Flush() during Reinitalize should defer the flush callback such that this call isn't possible until it completes.
was_reinitializing_sink_ = (state_ == kReinitializingSink);This should early return if state == kReinitializingSink and execute the flush_cb_ only when reinitailize completes.
DCHECK((state_ == kFlushed && !pending_read_) ||Shouldn't be possible after above advice.
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||Shouldn't be possible after above advice.
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||Shouldn't be possible after above advice.
}This needs to execute the flush process if flush_cb_ exists.
if ((state_ == kPlaying || state_ == kReinitializingSink) &&This should just early return if we're in the reinitializing state since playback is already paused for the sink switch -- it'll be called again if we still don't have enough data once playback starts on the new sink.
case kReinitializingSink:Probably this should be true to ensure we keep buffering while waiting for the sink change. It does mean you'd need to handle a case where a second config change comes in before the first completes though, so maybe too much trouble.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 14 comments![]( "Open in Gerrit")
Lets make this an else with the clause above.
Acknowledged
// Use input source audio channel count/layout if allowingDo we really need to keep just the channel count the same? I.e., if we prefer the stream sample rate, is that okay? I think it would let us drop this clause here and instead just add a new conditional to:
Without this change here, the output is locked to the default channel count, even when I add an additional conditional to the above "use_stream_params".
bool sink_in_playing_state_;Ditto, this should be removed.
Acknowledged
// True if |sink_| was undergoing reinitialization prior to playing.I don't think we need this new state. The point of having a `state_` variable is to avoid the need for additional bool multipliers on the state diagram.
Acknowledged
if ((state_ == kPlaying && was_reinitializing_sink_) ||Just early return with state == kReinitalizingSink.
Acknowledged
Likewise early return if state == reinitializing after setting sink_playing_ = false.
Acknowledged
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||This should only be state_ == kFlushed, Flush() during Reinitalize should defer the flush callback such that this call isn't possible until it completes.
Acknowledged
was_reinitializing_sink_ = (state_ == kReinitializingSink);This should early return if state == kReinitializingSink and execute the flush_cb_ only when reinitailize completes.
Acknowledged
DCHECK((state_ == kFlushed && !pending_read_) ||Shouldn't be possible after above advice.
Acknowledged
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||Shouldn't be possible after above advice.
Acknowledged
DCHECK(state_ == kFlushed || state_ == kReinitializingSink ||Shouldn't be possible after above advice.
Acknowledged
This needs to execute the flush process if flush_cb_ exists.
Acknowledged
if ((state_ == kPlaying || state_ == kReinitializingSink) &&This should just early return if we're in the reinitializing state since playback is already paused for the sink switch -- it'll be called again if we still don't have enough data once playback starts on the new sink.
Acknowledged
case kReinitializingSink:Probably this should be true to ensure we keep buffering while waiting for the sink change. It does mean you'd need to handle a case where a second config change comes in before the first completes though, so maybe too much trouble.
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 5 comments![]( "Open in Gerrit")
// Use input source audio channel count/layout if allowingMichael ChanDo we really need to keep just the channel count the same? I.e., if we prefer the stream sample rate, is that okay? I think it would let us drop this clause here and instead just add a new conditional to:
Without this change here, the output is locked to the default channel count, even when I add an additional conditional to the above "use_stream_params".
Hmm, that's surprising. Are you hitting this clause here:
Android should allow resampling passthrough, that combined with use_stream_params should mean you're able to use the stream params here.
BASE_FEATURE(kMatchSourceAudioChannelLayout,Can you file a bug for this feature that we can link to a "TODO(crbug.com/##): This should be replaced with a MediaClient mechanism if it works as intended"
} else {Initialize and return here since the sink should be stopped during Flush(). You can add a DCHECK(!sink_playing_) to confirm.
real_sink_needs_start_ = false;Do things work fine if you just copy the section from Initialize()? Maybe extract into a new InitializeSink() method that both can call?
```
sink_->Initialize(audio_parameters_, this);
if (null_sink_) {
null_sink_->Initialize(audio_parameters_, this);
null_sink_->Start(); // Does nothing but reduce state bookkeeping.
real_sink_needs_start_ = true;
} else {
// Even when kSuspendMutedAudio is enabled, we can hit this path if we are
// exclusively using NullAudioSink due to OnDeviceInfoReceived() failure.
sink_->Start();
sink_->Pause(); // Sinks play on start.
}
SetVolume(volume_);
```
sink_->Pause(); // Avoid issuing play if already playing.I think you can drop this if you switch to the method I suggested above.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 5 comments![]( "Open in Gerrit")
// Use input source audio channel count/layout if allowingMichael ChanDo we really need to keep just the channel count the same? I.e., if we prefer the stream sample rate, is that okay? I think it would let us drop this clause here and instead just add a new conditional to:
Dale CurtisWithout this change here, the output is locked to the default channel count, even when I add an additional conditional to the above "use_stream_params".
Hmm, that's surprising. Are you hitting this clause here:
Android should allow resampling passthrough, that combined with use_stream_params should mean you're able to use the stream params here.
I am not getting into the else part because expecting_config_changes_ gets set to 0 and hence use_stream_params gets set to 1.
BASE_FEATURE(kMatchSourceAudioChannelLayout,Can you file a bug for this feature that we can link to a "TODO(crbug.com/##): This should be replaced with a MediaClient mechanism if it works as intended"
I've added this issue in the tracker: https://issues.chromium.org/u/1/issues/445215599
Will add a TODO here.
} else {Initialize and return here since the sink should be stopped during Flush(). You can add a DCHECK(!sink_playing_) to confirm.
Acknowledged
real_sink_needs_start_ = false;Do things work fine if you just copy the section from Initialize()? Maybe extract into a new InitializeSink() method that both can call?
```
sink_->Initialize(audio_parameters_, this);
if (null_sink_) {
null_sink_->Initialize(audio_parameters_, this);
null_sink_->Start(); // Does nothing but reduce state bookkeeping.
real_sink_needs_start_ = true;
} else {
// Even when kSuspendMutedAudio is enabled, we can hit this path if we are
// exclusively using NullAudioSink due to OnDeviceInfoReceived() failure.
sink_->Start();
sink_->Pause(); // Sinks play on start.
}
SetVolume(volume_);
```
I will factor out the code you suggested into an InitializeSink() method and call it in the flush case. It doesn't work if I replace the code here with InitializeSink() as the semantics are slightly different: both sinks need to be initialized and Play() needs to be invoked, otherwise, playback doesn't commence after reinitialization.
sink_->Pause(); // Avoid issuing play if already playing.I think you can drop this if you switch to the method I suggested above.
I couldn't get it working with the method you suggested. Please refer to my comment above.
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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 2 comments![]( "Open in Gerrit")
real_sink_needs_start_ = false;Michael ChanDo things work fine if you just copy the section from Initialize()? Maybe extract into a new InitializeSink() method that both can call?
```
sink_->Initialize(audio_parameters_, this);
if (null_sink_) {
null_sink_->Initialize(audio_parameters_, this);
null_sink_->Start(); // Does nothing but reduce state bookkeeping.
real_sink_needs_start_ = true;
} else {
// Even when kSuspendMutedAudio is enabled, we can hit this path if we are
// exclusively using NullAudioSink due to OnDeviceInfoReceived() failure.
sink_->Start();
sink_->Pause(); // Sinks play on start.
}
SetVolume(volume_);
```
I will factor out the code you suggested into an InitializeSink() method and call it in the flush case. It doesn't work if I replace the code here with InitializeSink() as the semantics are slightly different: both sinks need to be initialized and Play() needs to be invoked, otherwise, playback doesn't commence after reinitialization.
The InitializeSink() you extracted does initialize both sinks if they exist. The call to SetVolume() should ensure we play the sink too if volume > 1.
Which clause of SetVolume() are we failing? I was expecting it would end up here:
Which should then end up here:
Due to setting `real_sink_needs_start_` to true in InitializeSink().
Which of the above isn't happening?
{I think you want to do this before you call DoFlush_Locked(). You can then clean up the auto lockers such that you only have a auto lock for state changes.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 2 comments![]( "Open in Gerrit")
Here's some more data:
09-17 09:39:33.762 6092 6143 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:889] SetVolume: mzchan: real_sink_needs_start_:1 volume_:1 volume:1 render_muted_audio_:0
09-17 09:39:33.762 6092 6143 I chromium: [INFO:media/renderers/audio_renderer_impl.cc:1684] MaybeStartRealSink: mzchan: real_sink_needs_start_:1 sink_playing_:0
Since sink_playing_ isn't set to true, sink_->Play() doesn't get triggered.
I think you want to do this before you call DoFlush_Locked(). You can then clean up the auto lockers such that you only have a auto lock for state changes.
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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")
Something seems wrong is `sink_playing_` is false. Do you know what's setting that? That means StopRendering must have been called.
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
This is probably due to a sink reinitialization occurring in the preroll stage before pressing the play button. I get these logs when loading up the media without pressing play:
09-18 09:34:17.960 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:382] Initialize
09-18 09:34:17.964 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:476] OnDeviceInfoReceived
09-18 09:34:17.964 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:509] OnDeviceInfoReceived: format: PCM_LOW_LATENCY, channel_layout: 3, channels: 2, sample_rate: 48000, frames_per_buffer: 1056, effects: NONE, mic_positions: , latency_tag: LatencyUnknown, hw_capabilities: min_frames_per_buffer: 0, max_frames_per_buffer: 0, bitstream_formats:0, require_encapsulation:0, require_audio_offload:0
09-18 09:34:17.965 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:689] OnDeviceInfoReceived: is_passthrough_=0 codec=eac3 stream->audio_decoder_config().sample_format=2
09-18 09:34:17.980 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:728] OnAudioDecoderStreamInitialized: 1
09-18 09:34:17.980 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1564] ChangeState_Locked : 1 -> 3
09-18 09:34:17.982 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:199] SetMediaTime(0 s)
09-18 09:34:17.982 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:365] StartPlaying
09-18 09:34:17.982 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1564] ChangeState_Locked : 3 -> 5
09-18 09:34:17.982 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1262] SetPlaybackRate(0)
09-18 09:34:17.989 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1009] DecodedAudioReady: Reconfiguring sink for channel count from 6 to 2
09-18 09:34:17.989 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1564] ChangeState_Locked : 5 -> 4
09-18 09:34:18.024 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:443] OnSourceChannelCountChanged: Reconfiguring sink for channel count:2
09-18 09:34:18.024 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1564] ChangeState_Locked : 4 -> 5
09-18 09:34:32.926 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:1262] SetPlaybackRate(0)
09-18 09:34:32.927 13012 13059 V chromium: [VERBOSE1:media/renderers/audio_renderer_impl.cc:104] ~AudioRendererImpl
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
Ah, in that case it should be okay for us to not start the new sink? It should start when playback starts up?
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
For this case maybe we don't need to start the sink but for the case when sink reinitializes during playback, I think it would be required because the sink was previously stopped. Not sure how to differentiate between these two cases though.
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
The SetVolume() call I linked above should restart the sinks though? Do you not see that happening?
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Michael Chan (Gerrit)
Michael Chan added 1 comment![]( "Open in Gerrit")
In Patchset 12, I've cleaned up the code and now I don't start the new sink. Playback starts and sink reinitialization works as expected. Only issue I see now is that when I mute the audio, playback stops. If I unmute, playback continues. Any ideas what might be happening?
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Chan (Gerrit)
Michael Chan added 3 comments![]( "Open in Gerrit")
Hi Dale,
I've addressed your comments and the code is in a good working state from my perspective. Please let me know what you think.
Regards, Michael
The mute/unmute issue is not seen anymore after I fixed a bug in NullAudioSink where the playing flag didn't get reset on Stop().
sink_->Pause(); // Avoid issuing play if already playing.I think you can drop this if you switch to the method I suggested above.
I couldn't get it working with the method you suggested. Please refer to my comment above.
This is not needed and has been removed now.
Related details
- Dale Curtis
- Dean Wheatley
- Maya Jelonkiewicz
- Thomas Guilbert
Code-Coverage
Code-Owners
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")
Great, can you add a small unit test to NullAudioSink for that?
Related details
- Dean Wheatley
- Maya Jelonkiewicz
- Michael Chan
- Thomas Guilbert
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
AttemptRead_Locked();Is this needed? I'd expect SetVolume() to start the sink going which should end up triggering this automatically.
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
Hi Dale,
I've addressed your comments in the latest patchset, but I have held off on null sink handling for now because I am getting DCHECK failures in null_audio_sink.cc.
Nevertheless, I am still getting the same DCHECK failure in web_audio_source_provider_impl.cc:
08-21 07:14:43.753 15030 15110 F DEBUG : Abort message: '[FATAL:third_party/blink/renderer/platform/media/web_audio_source_provider_impl.cc:298] DCHECK failed: state_ == kStarted (0 vs. 1)'
Michael ChanHmm, I'm not getting that same DCHECK over here. Are you able to reproduce that with https://storage.googleapis.com/dalecurtis/mse.html?src=aac_change_layout.adts&type=adts ? If so what are your repro steps?
Dale CurtisI am unable to start playback of that asset - it shows a spinning play icon and pressing play doesn't start playback and the icon continues to spin.
With the Dolby Digital Plus asset, I can play through the first time but again, waiting like ~60 seconds before attempting to play again will cause the DCHECK failure above on AAOS15 emulator.
Michael ChanHmm, the asset is just AAC. Are you building with proprietary_codecs=true and ffmpeg_branding=Chrome?
Dale CurtisYes, I have those flags in my args.gn:
target_os = "android"
target_cpu = "x64" # See "Figuring out target_cpu" below
android_static_analysis = "build_server"
proprietary_codecs=true
enable_platform_ac3_eac3_audio=true
ffmpeg_branding="Chrome"
is_debug=true
dcheck_always_on = true
That all looks right. I'm not sure why the linked stream wouldn't work for you then. I'll try to poke at this more tomorrow when I have more time. That sink state does mean that there's some missing state case not being handled.
Acknowledged
// Use input source audio channel count/layout if allowingDo we really need to keep just the channel count the same? I.e., if we prefer the stream sample rate, is that okay? I think it would let us drop this clause here and instead just add a new conditional to:
Dale CurtisWithout this change here, the output is locked to the default channel count, even when I add an additional conditional to the above "use_stream_params".
Michael ChanHmm, that's surprising. Are you hitting this clause here:
Android should allow resampling passthrough, that combined with use_stream_params should mean you're able to use the stream params here.
I am not getting into the else part because expecting_config_changes_ gets set to 0 and hence use_stream_params gets set to 1.
use_stream_params=true is what we want though? What params do we actually send to Initialize() in this case?