Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804493002:
Introduce a FLUSHED state in media::RendererImpl (Closed)
DescriptionIntroduce a FLUSHED state in media::RendererImpl
The renderer will be in the FLUSHED state after initialization and
after Flush completion, indicating that it's ready to start playback
after StartPlayingFrom call.
Review-Url: https://codereview.chromium.org/2804493002
Cr-Commit-Position: refs/heads/master@{#462592}
Committed: https://chromium.googlesource.com/chromium/src/+/a1597ec0c31e6d96f3fdba7c34c4d1b28dc52b4f
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase to ToT + use PostTask in Flush #
Total comments: 2
Messages
Total messages: 20 (13 generated)
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Description was changed from ========== Introduce STATE_FLUSHED for RendererImpl Renderer is in flushed state after init/flush, but before StartPlayingFrom is called. BUG= ========== to ========== Introduce STATE_FLUSHED for RendererImpl Renderer is in flushed state after init/flush, but before StartPlayingFrom is called. BUG= ==========
servolk@chromium.org changed reviewers: + xhwang@chromium.org
looking pretty good. just a few comments. https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:192: flush_cb.Run(); Directly run the callback could cause reentrancy issues if the caller isn't carefully implemetned, e.g. Step1(); Flush(base::Bind(&RendererHost::Bar, this)); Step2(); That's why traditionally we require all callbacks to be returned asynchronously (which is why we have media::BindToCurrentLoop). Recently we are more lenient with this as long as the caller is implemented correctly (to avoid unnecessary posts). However, in this case the caller isn't super clear since it's the SerialRunner: https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=875e8893... With that does it make sense to post the callback? https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); oh, this is based on your other CL? I thought you want to land this first.? https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.h:82: STATE_FLUSHED, Please add a comment. Otherwise it's not clear this is also the state after initialization finished.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.cc (right): https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:192: flush_cb.Run(); On 2017/04/06 05:20:28, xhwang_slow wrote: > Directly run the callback could cause reentrancy issues if the caller isn't > carefully implemetned, e.g. > > Step1(); > Flush(base::Bind(&RendererHost::Bar, this)); > Step2(); > > That's why traditionally we require all callbacks to be returned asynchronously > (which is why we have media::BindToCurrentLoop). Recently we are more lenient > with this as long as the caller is implemented correctly (to avoid unnecessary > posts). However, in this case the caller isn't super clear since it's the > SerialRunner: > > https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=875e8893... > > With that does it make sense to post the callback? Yes, I agree, PostTask should be safer here. Fixed. https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.cc:211: base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_)); On 2017/04/06 05:20:28, xhwang_slow wrote: > oh, this is based on your other CL? I thought you want to land this first.? I have initially create it based on my other CL, to ensure that it solves the issues discovered there, but now I've rebased the latest patchset to the clean Chromium master branch. https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... File media/renderers/renderer_impl.h (right): https://codereview.chromium.org/2804493002/diff/1/media/renderers/renderer_im... media/renderers/renderer_impl.h:82: STATE_FLUSHED, On 2017/04/06 05:20:28, xhwang_slow wrote: > Please add a comment. Otherwise it's not clear this is also the state after > initialization finished. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Introduce STATE_FLUSHED for RendererImpl Renderer is in flushed state after init/flush, but before StartPlayingFrom is called. BUG= ========== to ========== Introduce a FLUSHED state in media::RendererImpl The renderer will be in the FLUSHED state after initialization and after Flush completion, indicating that it's ready to start playback after StartPlayingFrom call. ==========
lg, just one question on the test https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl_unittest.cc:245: } Now it's not guaranteed that Flush() will Flush() the audio/video stream, maybe we should fix the expectation here so that Flush() on audio/video streams are optional? Or we can store a boolean about whether RendererImpl is already flushed... WDYT?
https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... File media/renderers/renderer_impl_unittest.cc (right): https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... media/renderers/renderer_impl_unittest.cc:245: } On 2017/04/06 17:42:55, xhwang_slow wrote: > Now it's not guaranteed that Flush() will Flush() the audio/video stream, maybe > we should fix the expectation here so that Flush() on audio/video streams are > optional? Or we can store a boolean about > whether RendererImpl is already flushed... WDYT? Audio/video renderers won't be flushed only when calling Flush immediately after renderer initialization or after another Flush, without calling StartPlayingFrom first (that's what the FlushAfterInitialization test does). In all other cases a/v renderers are guaranteed to be flushed. I believe the FlushAfterInitialization is a relatively rare corner case, I don't expect something like that to happen in other tests, so it's probably not worth special handling here, that's why I've just modified the FlushAfterInitialization to call RendererImpl::Flush directly, instead of modifying this Flush (and complicating the logic for the majority of other test cases).
On 2017/04/06 17:51:32, servolk wrote: > https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... > File media/renderers/renderer_impl_unittest.cc (right): > > https://codereview.chromium.org/2804493002/diff/20001/media/renderers/rendere... > media/renderers/renderer_impl_unittest.cc:245: } > On 2017/04/06 17:42:55, xhwang_slow wrote: > > Now it's not guaranteed that Flush() will Flush() the audio/video stream, > maybe > > we should fix the expectation here so that Flush() on audio/video streams are > > optional? Or we can store a boolean about > > whether RendererImpl is already flushed... WDYT? > > Audio/video renderers won't be flushed only when calling Flush immediately after > renderer initialization or after another Flush, without calling StartPlayingFrom > first (that's what the FlushAfterInitialization test does). In all other cases > a/v renderers are guaranteed to be flushed. I believe the > FlushAfterInitialization is a relatively rare corner case, I don't expect > something like that to happen in other tests, so it's probably not worth special > handling here, that's why I've just modified the FlushAfterInitialization to > call RendererImpl::Flush directly, instead of modifying this Flush (and > complicating the logic for the majority of other test cases). It makes sense to me. Thanks! LGTM
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491502820775370,
"parent_rev": "b84a182dfd565ca8080e0c51dd181f88a0a9452e", "commit_rev":
"a1597ec0c31e6d96f3fdba7c34c4d1b28dc52b4f"}
Message was sent while issue was closed.
Description was changed from ========== Introduce a FLUSHED state in media::RendererImpl The renderer will be in the FLUSHED state after initialization and after Flush completion, indicating that it's ready to start playback after StartPlayingFrom call. ========== to ========== Introduce a FLUSHED state in media::RendererImpl The renderer will be in the FLUSHED state after initialization and after Flush completion, indicating that it's ready to start playback after StartPlayingFrom call. Review-Url: https://codereview.chromium.org/2804493002 Cr-Commit-Position: refs/heads/master@{#462592} Committed: https://chromium.googlesource.com/chromium/src/+/a1597ec0c31e6d96f3fdba7c34c4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a1597ec0c31e6d96f3fdba7c34c4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
