Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269313002:
Fix DCHECK when track status changes in non-playing state (Closed)
DescriptionFix DCHECK when track status changes in non-playing state
We are only accessing shared_state_.renderer in kPlaying state.
BUG=640362
Committed: https://crrev.com/5f450aa795c85f615ccaed457f3114c487df7ac4
Cr-Commit-Position: refs/heads/master@{#416131}
Patch Set 1 #Patch Set 2 : Fixed one more DCHECK + added a unit test #
Total comments: 5
Patch Set 3 : Added a unit test for track changes in suspended state #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== Fix DCHECK when track status changes in non-playing state BUG=640362 ========== to ========== Fix DCHECK when track status changes in non-playing state BUG=640362 ==========
servolk@chromium.org changed reviewers: + avayvod@chromium.org, chcunningham@chromium.org
Description was changed from ========== Fix DCHECK when track status changes in non-playing state BUG=640362 ========== to ========== Fix DCHECK when track status changes in non-playing state We are only accessing shared_state_.renderer in kPlaying state. BUG=640362 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2269313002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2269313002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:232: if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_)) This seems to be a separate issue from relaxing that DCHECK? Can you explain the circumstances here? https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1027: ASSERT_TRUE(WaitUntilOnEnded()); Is the expectation that waiting until ended will cause the renderer to be removed due to suspend? IIRC it takes a few seconds for suspend to kick in -- you may not be reliably testing the DCHECK case
https://codereview.chromium.org/2269313002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2269313002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl.cc:232: if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_)) On 2016/08/24 22:16:53, chcunningham wrote: > This seems to be a separate issue from relaxing that DCHECK? Can you explain the > circumstances here? Yes, somewhat separate, but related. First I've simply updated the DCHECK condition (see patchset #1). But then I decided to add a unit test that would test changing media track status while the pipeline is not in the kPlaying state (previously it would trip on that DCHECK). But when I added a unit test that waits for playback to end (so that pipeline is in the stopped state) and then changes tracks, it would hit a different DCHECK (here https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=0&l=552). It turns out that RendererImpl stays in the STATE_PLAYING even after both renderers are ended. So we would get here and would try to call RestartA/VRenderer below even though the playback of both streams has already ended. By adding this additional check we'll ensure that media track status changes when the RendererImpl has been ended, we won't try to restart renderers. Alternatively I guess we could try to set RendererImpl::state_ to something else besides STATE_PLAYING in RendererImpl::RunEndedCallbackIfNeeded when we detect that playback has actually ended. But none of the other RendererImpl::State enum values seem to be fitting this situation, and I wanted to keep this change small and safe. So I've chose to modify the check here, rather than mess with state_ or introduce a new State enum value. https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1027: ASSERT_TRUE(WaitUntilOnEnded()); On 2016/08/24 22:16:53, chcunningham wrote: > Is the expectation that waiting until ended will cause the renderer to be > removed due to suspend? IIRC it takes a few seconds for suspend to kick in -- > you may not be reliably testing the DCHECK case No, we don't need suspend to happen. After pipeline is ended it transitions into the kStopped state and that is enough to test that the updated DCHECK in the pipeline_impl.cc is no longer triggered.
https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1027: ASSERT_TRUE(WaitUntilOnEnded()); I don't think this transition to stopped is happening. I patched your change and added logs to pipeline_impl just before your dcheck: OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 So the state is always 3 (playing) and the renderer pointer is always non-null. So this test passes before and after your changes to the DCHECK.
On 2016/08/25 20:45:54, chcunningham wrote: > https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... > File media/test/pipeline_integration_test.cc (right): > > https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... > media/test/pipeline_integration_test.cc:1027: ASSERT_TRUE(WaitUntilOnEnded()); > I don't think this transition to stopped is happening. I patched your change and > added logs to pipeline_impl just before your dcheck: > > OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > > So the state is always 3 (playing) and the renderer pointer is always non-null. > So this test passes before and after your changes to the DCHECK. Oops, you are right, the new test is passing even without changes to pipeline_impl.cc (but it crashes on a DCHECK without changes to renderer_impl.cc). I'll see if I can find a way to get the pipeline impl in a state different than kPlaying in this test.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/29 17:44:41, servolk wrote: > On 2016/08/25 20:45:54, chcunningham wrote: > > > https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... > > File media/test/pipeline_integration_test.cc (right): > > > > > https://codereview.chromium.org/2269313002/diff/20001/media/test/pipeline_int... > > media/test/pipeline_integration_test.cc:1027: ASSERT_TRUE(WaitUntilOnEnded()); > > I don't think this transition to stopped is happening. I patched your change > and > > added logs to pipeline_impl just before your dcheck: > > > > OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > > OnEnabledAudioTracksChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > > OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > > OnSelectedVideoTrackChanged shared_state_.renderer:0x381e5dca8e20 state_:3 > > > > So the state is always 3 (playing) and the renderer pointer is always > non-null. > > So this test passes before and after your changes to the DCHECK. > > Oops, you are right, the new test is passing even without changes to > pipeline_impl.cc (but it crashes on a DCHECK without changes to > renderer_impl.cc). I'll see if I can find a way to get the pipeline impl in a > state different than kPlaying in this test. Ok, I've looked a bit more into this and added a new unit test in the latest patchset. In this new unit test we'll perform track status changes in the suspended state and I've verified that it would trip on the DCHECK in the pipeline_impl.cc if we don't update it with the right condition. I've also kept the change in the renderer_impl.cc and the unit test for track status changes after pipeline has ended, since those are useful too. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2269313002/#ps40001 (title: "Added a unit test for track changes in suspended state")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix DCHECK when track status changes in non-playing state We are only accessing shared_state_.renderer in kPlaying state. BUG=640362 ========== to ========== Fix DCHECK when track status changes in non-playing state We are only accessing shared_state_.renderer in kPlaying state. BUG=640362 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix DCHECK when track status changes in non-playing state We are only accessing shared_state_.renderer in kPlaying state. BUG=640362 ========== to ========== Fix DCHECK when track status changes in non-playing state We are only accessing shared_state_.renderer in kPlaying state. BUG=640362 Committed: https://crrev.com/5f450aa795c85f615ccaed457f3114c487df7ac4 Cr-Commit-Position: refs/heads/master@{#416131} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5f450aa795c85f615ccaed457f3114c487df7ac4 Cr-Commit-Position: refs/heads/master@{#416131}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2310483002/ by servolk@chromium.org. The reason for reverting is: The new test is crashing on Mac. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
