Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1641423002:
Re-land extract state management from WebMediaPlayerImpl. (Closed)
|
Created:
4 years, 10 months ago by sandersd (OOO until July 31) Modified:
4 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, halliwell, xhwang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract state management from WebMediaPlayerImpl.
WMPI recieves operations at any time but is backed by Pipeline which can
only perform one operation at a time. This CL creates a helper class
(PipelineController) to translate between the two so that WMPI does not need
to handle the full cross product of states.
Committed: https://crrev.com/1c0bba0d8cbd1390822796b22bfa9bc353d4b5c8
Cr-Commit-Position: refs/heads/master@{#379386}
Patch Set 1 #Patch Set 2 : WMPI fixes. #
Total comments: 20
Patch Set 3 : Rename to PipelineController. #Patch Set 4 : Move to media/filters. #Patch Set 5 : Rebase on PipelineImpl #Patch Set 6 : Add unittest. #Patch Set 7 : Rebase. #Patch Set 8 : Fix scoped_ptr usage. #
Total comments: 4
Patch Set 9 : Rebase. #Patch Set 10 : Rebase again. #Patch Set 11 : Remove remaining uses of base::Unretained(). #
Total comments: 9
Patch Set 12 : Fix cast. #Patch Set 13 : Make OnPipelineSeeked() public. #Patch Set 14 : More unit tests. #
Total comments: 26
Patch Set 15 : BindToCurrentLoop #Patch Set 16 : Add a message loop to the test. #
Total comments: 18
Patch Set 17 : Address comments. #Patch Set 18 : Add thread checker. #
Total comments: 4
Patch Set 19 : Fix binding in WMPI. #Patch Set 20 : Typo #Patch Set 21 : Rebase. #Patch Set 22 : Rebase #Patch Set 23 : Log all the state transitions. #Patch Set 24 : Log onTimeUpdated. #Patch Set 25 : Rebase. #Patch Set 26 : Don't call play until after setting the src. #Patch Set 27 : Add logging to PipelineController. #Patch Set 28 : Fix test failure. #Patch Set 29 : Rebase. #Patch Set 30 : Add blink logging. #Patch Set 31 : Rebase #Patch Set 32 : Remove debugging code. #Patch Set 33 : Initialize |should_notify_time_changed_|. #Messages
Total messages: 111 (47 generated)
sandersd@chromium.org changed reviewers: + dalecurtis@chromium.org, wolenetz@chromium.org
This CL is not production ready yet (in particular no tests have been run, no new tests have been written, and pointer lifetime is not managed properly). However, it's a big change to WMPI so I'm looking for feedback on the overall design before those details are settled.
Seems reasonable to me. I'm always happy to see less code in WMPI. Especially if we get unittests for the pipeline state class. Naming wise, it's not just a state manager. Perhaps it should be called PipelineController or something similar? I'm slightly torn though since PipelineState is starting to look a lot like a Pipeline... should we instead just merge all of this into Pipeline itself? It smells a little to add an extra layer of indirection here. cc:xhwang in case he has opinions.
I look forward to some tests included in this. Some intro nits/questions: https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... File media/blink/pipeline_state.cc (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:62: // not apply to MSE because the underlying buffer could have been changed nit: as you mentioned yesterday, this could also apply to file-backed blob urls, backed by a potentially changing file (impacting multibuffer work, too) https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:122: // Note: Dispatch() may be called twice in a row. (See OnPipelineStatus().) nit: Saying it must support re-entrancy by (at least) OnPipelineStatus() would be more clear. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:129: pipeline_->Suspend(base::Bind(&PipelineState::OnPipelineStatus, In the case of an already-in-progress pipeline_->Seek(...), does pipeline_->Suspend(...) do the right thing? For instance, do we need to do anything do enforce immediate renderer suspend independently from canceling or pending further on a potentially blocked-pending-buffers-at-target-seek operation from completing? https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1082: DoSeek(base::TimeDelta::FromSecondsD(t), false); Do we need to do anything with paused_time_/seek_time_ in this case (which IIUC is specific to WMPI cast)? https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1083: if (delegate_ && !delegate_->IsHidden()) I confess I don't understand what's going on here w.r.t. delegate_. Can you explain please?
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I didn't review details. Just general comments. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... File media/blink/pipeline_state.h (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:6: #define MEDIA_BLINK_PIPELINE_STATE_H_ Can we put this file in the same folder as the pipeline.*? It seems it doesn't depend on anything related to blink. Note that pipeline.* is currently in media/base which probably isn't the ideal place. But that's a separate issue. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:33: class PipelineState { Can we hide this in the Pipeline so that WMPI only need to deal with one Pipeline which has this simpler API? Actually it seems the PipelineState is the new Pipeline, and the Pipeline is more like a PipelineImpl. Do we really need to expose the old Pipeline to WMPI?
https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... File media/blink/pipeline_state.cc (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:54: // TODO(sandersd): Is it even possible to have an elided seek during Start()? FYI: I removed this TODO, because it turns out that we want that ability for Resume(), even if Start() isn't affected. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:62: // not apply to MSE because the underlying buffer could have been changed On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > nit: as you mentioned yesterday, this could also apply to file-backed blob urls, > backed by a potentially changing file (impacting multibuffer work, too) I added a comment about that for now. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:122: // Note: Dispatch() may be called twice in a row. (See OnPipelineStatus().) On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > nit: Saying it must support re-entrancy by (at least) OnPipelineStatus() would > be more clear. Done. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.cc:129: pipeline_->Suspend(base::Bind(&PipelineState::OnPipelineStatus, On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > In the case of an already-in-progress pipeline_->Seek(...), does > pipeline_->Suspend(...) do the right thing? For instance, do we need to do > anything do enforce immediate renderer suspend independently from canceling or > pending further on a potentially blocked-pending-buffers-at-target-seek > operation from completing? While I tested this manually, and it seemed to work, you're correct that there seems to be a logical hole where the resume time would not account for the seek that was cancelled. I've added new logic to handle that case by turning the current seek back into a pending seek. PTAL. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... File media/blink/pipeline_state.h (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/01 21:16:44, xhwang wrote: > nit: 2016 Done. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:6: #define MEDIA_BLINK_PIPELINE_STATE_H_ On 2016/02/01 21:16:44, xhwang wrote: > Can we put this file in the same folder as the pipeline.*? It seems it doesn't > depend on anything related to blink. > > Note that pipeline.* is currently in media/base which probably isn't the ideal > place. But that's a separate issue. It depends on media/filters; I wasn't sure if that is allowed. https://codereview.chromium.org/1641423002/diff/20001/media/blink/pipeline_st... media/blink/pipeline_state.h:33: class PipelineState { On 2016/02/01 21:16:44, xhwang wrote: > Can we hide this in the Pipeline so that WMPI only need to deal with one > Pipeline which has this simpler API? > > Actually it seems the PipelineState is the new Pipeline, and the Pipeline is > more like a PipelineImpl. Do we really need to expose the old Pipeline to WMPI? > That's exactly the plan (pending some changes to the Demuxer interface so that we can drop the ChunkDemuxer dependency). However, I only wanted to tackle one refactor at a time, and so I have this in-between CL. https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1082: DoSeek(base::TimeDelta::FromSecondsD(t), false); On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > Do we need to do anything with paused_time_/seek_time_ in this case (which IIUC > is specific to WMPI cast)? We do; it should be done correctly by DoSeek(). In fact the main reason I separated seek() from DoSeek() was so that that logic would not be duplicated here anymore. (Although I am still suspicious of some of the cast state manipulations; we may want to review them separately at some point.) https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1083: if (delegate_ && !delegate_->IsHidden()) On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > I confess I don't understand what's going on here w.r.t. delegate_. Can you > explain please? This just says that we want to resume playback when casting ends (it should end up in a paused state). However, if we are not the foreground tab then we would be suspended anyway, so don't resume.
Can you add some arguments for/against why we shouldn't just roll this into Pipeline?
https://chromiumcodereview.appspot.com/1641423002/diff/20001/media/blink/pipe... File media/blink/pipeline_state.h (right): https://chromiumcodereview.appspot.com/1641423002/diff/20001/media/blink/pipe... media/blink/pipeline_state.h:6: #define MEDIA_BLINK_PIPELINE_STATE_H_ On 2016/02/01 23:19:26, sandersd wrote: > On 2016/02/01 21:16:44, xhwang wrote: > > Can we put this file in the same folder as the pipeline.*? It seems it doesn't > > depend on anything related to blink. > > > > Note that pipeline.* is currently in media/base which probably isn't the ideal > > place. But that's a separate issue. > > It depends on media/filters; I wasn't sure if that is allowed. Then probably we should put this under media/filters, and then move pipeline.* over in the future as well. +dalecurtis
On 2016/02/01 23:24:05, DaleCurtis wrote:
> Can you add some arguments for/against why we shouldn't just roll this into
> Pipeline?
Because the current plan is to eventually roll this into Pipeline as a second
refactor, some of the against reasons are temporary. Given that:
Against:
- Will take longer to prepare, test, and commit this CL. During that time, the
WMPI state machine continues to be hard to understand and work with.
- watk@ has pending changes to the suspend/resume state machine.
- The API seen by WMPI should be the same either way. Having this intermediate
state makes the CLs easier to review.
- Doing the work separately makes it easier to see any subtle internal
dependencies (by preventing them from existing).
- Said differently, as a separate class the implementation cannot possibly
use any state fiddling hacks. The design is sure to be self-contained.
- It's easier to read and understand PipelineController by itself than the
same logic inside Pipeline.
- The existing pipeline API is simpler and may be easier to test on its own.
- PipelineController is simpler than Pipeline and would be easier to test on
its own, if Pipeline were virtual.
- [We should probably make Pipeline virtual.]
- Requires changing the Demuxer API or making Pipeline depend on ChunkDemuxer
explicitly.
For:
- Less churn overall.
- Less indirection.
- No confusion over which class to use (in general and per-call).
- Removes duplication, such as the way that PipelineController approximately
tracks the |state_| of Pipeline.
- Less code to read and understand to understand all of Pipeline.
- Could simplify test writing, since Pipeline calls can be queued.
Now that I've written all that, the against side looks surprisingly compelling.
Maybe we really do want to fully encapsulate Pipeline but not actually merge
them?
On 2016/02/01 at 23:52:25, sandersd wrote: > On 2016/02/01 23:24:05, DaleCurtis wrote: > > Can you add some arguments for/against why we shouldn't just roll this into > > Pipeline? > > Because the current plan is to eventually roll this into Pipeline as a second refactor, some of the against reasons are temporary. Given that: > > Against: > - Will take longer to prepare, test, and commit this CL. During that time, the WMPI state machine continues to be hard to understand and work with. > - watk@ has pending changes to the suspend/resume state machine. > - The API seen by WMPI should be the same either way. Having this intermediate state makes the CLs easier to review. > - Doing the work separately makes it easier to see any subtle internal dependencies (by preventing them from existing). > - Said differently, as a separate class the implementation cannot possibly use any state fiddling hacks. The design is sure to be self-contained. > - It's easier to read and understand PipelineController by itself than the same logic inside Pipeline. > - The existing pipeline API is simpler and may be easier to test on its own. > - PipelineController is simpler than Pipeline and would be easier to test on its own, if Pipeline were virtual. > - [We should probably make Pipeline virtual.] > - Requires changing the Demuxer API or making Pipeline depend on ChunkDemuxer explicitly. > > For: > - Less churn overall. > - Less indirection. > - No confusion over which class to use (in general and per-call). > - Removes duplication, such as the way that PipelineController approximately tracks the |state_| of Pipeline. > - Less code to read and understand to understand all of Pipeline. > - Could simplify test writing, since Pipeline calls can be queued. > > Now that I've written all that, the against side looks surprisingly compelling. Maybe we really do want to fully encapsulate Pipeline but not actually merge them? As discussed offline, I'm not against doing a two-stage refactor if you find that will be simpler and result in a same clean end. My worry is that it would be harder overall to do it in two stages than to put the thought in upfront to expand Pipeline and its tests to our designed end state.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineState) to translate between the two so that WMPI does not need to handle the full cross product of states. ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ==========
So in the future world we'll have Pipeline (pure virtual interface), PipelineImpl (today's Pipeline), and PipelineController (extracting of existing state from WMPI)? https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:150: base::Unretained(this)), These probably still need to be WeakPtrs unless its guaranteed that none will fire after ~WMPI. https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:196: void OnPipelineSeeked(bool time_updated); Why are these public? Ditto for the rest of the OnXXX above?
PipelineController sgtm from naming's perspective. Dan and I chat about the future world as well. I wonder whether we could make the threading model cleaner. For example PipelineController lives exclusively on the main thread, and PipelineImpl lives exclusively on the media thread. Then we have a dedicated class, e.g. PipelineProxy, to help do the thread hopping. Some sync calls like GetCurrentMediaTime() could be hard to implement. There are some possibilities. For example, we can cache the media time and update it constantly (like what we do for WMPA). Or we can say that PipelineImpl is single threaded, except the GetCurrentMediaTime() call, which could be called on any thread. Not ideal, but should be acceptable.
Just some more unit tests to go, but they won't happen today. PTAL at the rest. https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:150: base::Unretained(this)), On 2016/02/19 23:28:30, DaleCurtis wrote: > These probably still need to be WeakPtrs unless its guaranteed that none will > fire after ~WMPI. Done. https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:196: void OnPipelineSeeked(bool time_updated); On 2016/02/19 23:28:30, DaleCurtis wrote: > Why are these public? Ditto for the rest of the OnXXX above? Done.
Didn't have time to make it through everything, but some initial comments. Overall looks good! https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:150: base::Unretained(this)), Why the mix of unretained w/ AsWeakPtr() here? https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a It's not clear on why you would call this |time_updated| vs |render_initiated_seek|. Can you elaborate or change the names here?
https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:150: base::Unretained(this)), On 2016/02/24 01:48:24, DaleCurtis wrote: > Why the mix of unretained w/ AsWeakPtr() here? CreateRenderer() is a factory method; you can't bind using weak pointers to methods that return values. There can't be a path that it is called on after |pipeline_controller_| is destructed (because all the callbacks there are using weak pointers), so this is safe. But annoyingly subtle. The suggestion in the Pipeline refactoring doc to switch to a client interface would make this somewhat simpler (by moving the burden inside PipelineController). https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a On 2016/02/24 01:48:24, DaleCurtis wrote: > It's not clear on why you would call this |time_updated| vs > |render_initiated_seek|. Can you elaborate or change the names here? Done. Would it make sense to plumb this change all the way through Pipeline?
https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a On 2016/02/25 at 00:16:26, sandersd wrote: > On 2016/02/24 01:48:24, DaleCurtis wrote: > > It's not clear on why you would call this |time_updated| vs > > |render_initiated_seek|. Can you elaborate or change the names here? > > Done. > > Would it make sense to plumb this change all the way through Pipeline? Do renderer's initiate seeks these day? On second thought I'm not sure what you mean by renderer initiated seek. I didn't think that was possible yet. https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:192: // Called from WebMediaPlayerCast. Mark with TODO WMPI_CAST make private? https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:197: Extra space? https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:301: PipelineImpl pipeline_; Might add a comment here that pipeline_controller_ references pipeline so thus must always be destructed before pipeline_. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.cc:48: DCHECK(state_ == State::CREATED); DCHECK_EQ https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:30: : public base::SupportsWeakPtr<PipelineController> { Do we need SupportsWeakPtr() here? Can you instead use a WeakFactory. I don't see external classes using this functionality. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:42: typedef base::Callback<scoped_ptr<Renderer>(void)> RendererFactoryCB; "using" instead of typedef now https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:46: PipelineController(Pipeline* pipeline, Needs comments about lifetime of pipeline and what each cb does. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:68: void Seek(base::TimeDelta time, bool time_updated); Ditto for comments. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller_unittest.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) these days. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:18: using ::testing::_; We generally elide the first :: https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:94: void OnSeeked(bool time_updated) {} Seems you might want to verify these two?
https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:150: base::Unretained(this)), On 2016/02/25 00:16:26, sandersd wrote: > On 2016/02/24 01:48:24, DaleCurtis wrote: > > Why the mix of unretained w/ AsWeakPtr() here? > > CreateRenderer() is a factory method; you can't bind using weak pointers to > methods that return values. The CDM creation in CdmFactory::Create() is async, which I find convenient. So if needed, we can switch the renderer creation from sync to async.
xhwang: I included my BindToCurrentLoop() change, PTAL at that and how it affects the test and let me know if it's too horrible. Note that in an ideal world we implement your suggestion to handle this thread hopping in a helper, and PipelineController wouldn't need to know about this at all (which would mean that the tests would look like they currently do). https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a On 2016/02/25 01:58:29, DaleCurtis wrote: > On 2016/02/25 at 00:16:26, sandersd wrote: > > On 2016/02/24 01:48:24, DaleCurtis wrote: > > > It's not clear on why you would call this |time_updated| vs > > > |render_initiated_seek|. Can you elaborate or change the names here? > > > > Done. > > > > Would it make sense to plumb this change all the way through Pipeline? > > Do renderer's initiate seeks these day? On second thought I'm not sure what you > mean by renderer initiated seek. I didn't think that was possible yet. They do, in two cases: On Resume() and when ending cast. I actually went through and reverted this change, it was terrible to have both |time_updated| and |renderer_initated| with opposite meanings. We should pick one and plumb it everywhere. (And if we decide the correct name is something else, I can do it in this CL, but I would rather do it after as a separate change.) https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:192: // Called from WebMediaPlayerCast. On 2016/02/25 01:58:29, DaleCurtis wrote: > Mark with TODO WMPI_CAST make private? Done. https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:197: On 2016/02/25 01:58:30, DaleCurtis wrote: > Extra space? The methods were distinguished by whether they were associated with PipelineController or Pipeline, but with OnPipelineSeeked() separated it doesn't make visual sense anymore. Done. https://codereview.chromium.org/1641423002/diff/260001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:301: PipelineImpl pipeline_; On 2016/02/25 01:58:30, DaleCurtis wrote: > Might add a comment here that pipeline_controller_ references pipeline so thus > must always be destructed before pipeline_. Done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.cc:48: DCHECK(state_ == State::CREATED); On 2016/02/25 01:58:30, DaleCurtis wrote: > DCHECK_EQ ../../base/logging.h:523:23: error: invalid operands to binary expression ('basic_ostream<char, std::char_traits<char> >' and 'media::PipelineController::State') https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:30: : public base::SupportsWeakPtr<PipelineController> { On 2016/02/25 01:58:30, DaleCurtis wrote: > Do we need SupportsWeakPtr() here? Can you instead use a WeakFactory. I don't > see external classes using this functionality. Done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:42: typedef base::Callback<scoped_ptr<Renderer>(void)> RendererFactoryCB; On 2016/02/25 01:58:30, DaleCurtis wrote: > "using" instead of typedef now Done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:46: PipelineController(Pipeline* pipeline, On 2016/02/25 01:58:30, DaleCurtis wrote: > Needs comments about lifetime of pipeline and what each cb does. Those comments made somewhat more sense on the member variables, it's a lot to take in to document the entire class for the constructor. So what I've done now is just a summary here. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.h:68: void Seek(base::TimeDelta time, bool time_updated); On 2016/02/25 01:58:30, DaleCurtis wrote: > Ditto for comments. Done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller_unittest.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/02/25 01:58:30, DaleCurtis wrote: > No (c) these days. Done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:18: using ::testing::_; On 2016/02/25 01:58:30, DaleCurtis wrote: > We generally elide the first :: That seems to be just you. (And separately I have experienced namespace collisions in Chromium media tests on the testing namespace specifically.) https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:94: void OnSeeked(bool time_updated) {} On 2016/02/25 01:58:30, DaleCurtis wrote: > Seems you might want to verify these two? Done.
All minor comments / nits except one around l.864 in WMPI.cc (though I'm unsure if the scenario in question is possible). Looking good. https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1082: DoSeek(base::TimeDelta::FromSecondsD(t), false); On 2016/02/01 23:19:26, sandersd wrote: > On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > > Do we need to do anything with paused_time_/seek_time_ in this case (which > IIUC > > is specific to WMPI cast)? > > We do; it should be done correctly by DoSeek(). > > In fact the main reason I separated seek() from DoSeek() was so that that logic > would not be duplicated here anymore. (Although I am still suspicious of some of > the cast state manipulations; we may want to review them separately at some > point.) Acknowledged. https://codereview.chromium.org/1641423002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1083: if (delegate_ && !delegate_->IsHidden()) On 2016/02/01 23:19:26, sandersd wrote: > On 2016/01/29 21:42:30, wolenetz_OoO_Feb_1 wrote: > > I confess I don't understand what's going on here w.r.t. delegate_. Can you > > explain please? > > This just says that we want to resume playback when casting ends (it should end > up in a paused state). However, if we are not the foreground tab then we would > be suspended anyway, so don't resume. Acknowledged. https://codereview.chromium.org/1641423002/diff/300001/media/base/mock_filters.h File media/base/mock_filters.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/base/mock_filter... media/base/mock_filters.h:72: // TODO(sandersd): These should probably have setters to. nit: too https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:864: // Ignore state changes until we've completed all outstanding operations. What if the pipeline ends while suspending or resuming request to it haven't reached it yet, but the pipelinecontroller knows it's being suspended or resumed? Will we drop the "pipeline ended" notification on the floor now versus prior to this change? https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.cc:51: Let's further enforce the contract that, if chunk_demuxer is set, then it must be the same demuxer as demuxer... e.g. DCHECK(!chunk_demuxer || (chunk_demuxer == demuxer)). This was previously done inside one method in WMPI, now it's split across two classes, so let's keep it clean :) Perhaps this can be further cleaned-up in that follow-up CL by just using a boolean to indicate if we're using MSE, or use two injected methods or something for those specific non-Demuxer methods that ChunkDemuxer needs to be signalled with by PipelineController in MSE case. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:18: class ChunkDemuxer; per chat, in follow-up CL, please find a way of testing the ChunkDemuxer-API surface area exercised by PipelineController. The specialization of seek behavior for ChunkDemuxer in combination with suspend/resume/pause/cast/etc. is a risky surface area to keep untested for long. Among several alternatives, one potentially simple one might be to identify which methods (StartWaitingForSeek(...),CancelPendingSeek(...)) of ChunkDemuxer that are not in Demuxer interface are exercised by PipelineController, and see if they can be elevated to the Demuxer interface with default no-op implementations there for non-ChunkDemuxers. Then, various for-testing methods on PipelineController can be exercised by the unit tests to make it think it's operating on a ChunkDemuxer, and verify that the StartWaitingForSeek/etc methods occur when they should (perhaps by injecting them as for-test callbacks). https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:97: bool IsStable(); aside: I like "stable state" concept here. Be wary, though, of potential confusion with HTMLMediaElement/Blink "stable state" concept, which is totally different :) https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:139: bool pending_seeked_ = false; nit: s/pending_seeked_/pending_seek_completed_/ to help prevent confusion with very similar |pending_seek_|.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Hit dry run for you. Overall looks good. https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a On 2016/02/25 at 20:33:45, sandersd wrote: > On 2016/02/25 01:58:29, DaleCurtis wrote: > > On 2016/02/25 at 00:16:26, sandersd wrote: > > > On 2016/02/24 01:48:24, DaleCurtis wrote: > > > > It's not clear on why you would call this |time_updated| vs > > > > |render_initiated_seek|. Can you elaborate or change the names here? > > > > > > Done. > > > > > > Would it make sense to plumb this change all the way through Pipeline? > > > > Do renderer's initiate seeks these day? On second thought I'm not sure what you > > mean by renderer initiated seek. I didn't think that was possible yet. > > They do, in two cases: On Resume() and when ending cast. > > I actually went through and reverted this change, it was terrible to have both |time_updated| and |renderer_initated| with opposite meanings. We should pick one and plumb it everywhere. > > (And if we decide the correct name is something else, I can do it in this CL, but I would rather do it after as a separate change.) That's not a media::Renderer initiated seek tough. Resume() nor ending Cast touches that level of code, hence the confusing name. Can you change the comment to something less confusing? https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:891: if (resuming_ && playback_rate_ > 0 && !paused_) See https://codereview.chromium.org/1739473003 -- I think you have some issues here that I fixed in that CL. You've deleted this chunk of code, but by doing that I think you're always signaling the delegates when in the suspended state, which will hold the power save blocker. That can happen if JS issues a play() call to a tag bestowed with the magical user gesture (or said restriction is disabled). You can test with this page http://storage.googleapis.com/dalecurtis-shared/bg_start.html https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:193: // TODO(hubbe): Make private. Please explicitly use the words WMPI_CAST so that its easily found via search.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/300001
https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.cc:48: DCHECK(state_ == State::CREATED); On 2016/02/25 at 20:33:46, sandersd wrote: > On 2016/02/25 01:58:30, DaleCurtis wrote: > > DCHECK_EQ > > ../../base/logging.h:523:23: error: invalid operands to binary expression ('basic_ostream<char, std::char_traits<char> >' and 'media::PipelineController::State') Hmm, that's weird can we not use DCHECK_EQ with an enum class? https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller_unittest.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:18: using ::testing::_; On 2016/02/25 at 20:33:46, sandersd wrote: > On 2016/02/25 01:58:30, DaleCurtis wrote: > > We generally elide the first :: > > That seems to be just you. > > (And separately I have experienced namespace collisions in Chromium media tests on the testing namespace specifically.) 3652 hits https://code.google.com/p/chromium/codesearch#search/&q=%5C%20testing::&sq=pa... 1626 hits https://code.google.com/p/chromium/codesearch#search/&q=%5C%20::testing::&sq=... Pretty sure it's not just me :) How would get collisions in the testing namespace? gtest/gmock, other stuff in /test/ is the only one using that namespace.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/200001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:204: // Actually seek. This is a regular seek if |time_updated| is true, and a On 2016/02/26 02:54:24, DaleCurtis wrote: > On 2016/02/25 at 20:33:45, sandersd wrote: > > On 2016/02/25 01:58:29, DaleCurtis wrote: > > > On 2016/02/25 at 00:16:26, sandersd wrote: > > > > On 2016/02/24 01:48:24, DaleCurtis wrote: > > > > > It's not clear on why you would call this |time_updated| vs > > > > > |render_initiated_seek|. Can you elaborate or change the names here? > > > > > > > > Done. > > > > > > > > Would it make sense to plumb this change all the way through Pipeline? > > > > > > Do renderer's initiate seeks these day? On second thought I'm not sure what > you > > > mean by renderer initiated seek. I didn't think that was possible yet. > > > > They do, in two cases: On Resume() and when ending cast. > > > > I actually went through and reverted this change, it was terrible to have both > |time_updated| and |renderer_initated| with opposite meanings. We should pick > one and plumb it everywhere. > > > > (And if we decide the correct name is something else, I can do it in this CL, > but I would rather do it after as a separate change.) > > That's not a media::Renderer initiated seek tough. Resume() nor ending Cast > touches that level of code, hence the confusing name. Can you change the comment > to something less confusing? I believe this was done. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller.cc:48: DCHECK(state_ == State::CREATED); On 2016/02/26 02:59:34, DaleCurtis wrote: > On 2016/02/25 at 20:33:46, sandersd wrote: > > On 2016/02/25 01:58:30, DaleCurtis wrote: > > > DCHECK_EQ > > > > ../../base/logging.h:523:23: error: invalid operands to binary expression > ('basic_ostream<char, std::char_traits<char> >' and > 'media::PipelineController::State') > > Hmm, that's weird can we not use DCHECK_EQ with an enum class? That is correct. https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... File media/filters/pipeline_controller_unittest.cc (right): https://codereview.chromium.org/1641423002/diff/260001/media/filters/pipeline... media/filters/pipeline_controller_unittest.cc:18: using ::testing::_; On 2016/02/26 02:59:34, DaleCurtis wrote: > On 2016/02/25 at 20:33:46, sandersd wrote: > > On 2016/02/25 01:58:30, DaleCurtis wrote: > > > We generally elide the first :: > > > > That seems to be just you. > > > > (And separately I have experienced namespace collisions in Chromium media > tests on the testing namespace specifically.) > > 3652 hits > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20testing::&sq=pa... > > > 1626 hits > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20::testing::&sq=... > > > Pretty sure it's not just me :) How would get collisions in the testing > namespace? gtest/gmock, other stuff in /test/ is the only one using that > namespace. https://code.google.com/p/chromium/codesearch#search/&q=%22using%20testing::%... (36 hits) https://code.google.com/p/chromium/codesearch#search/&q=%22using%20::testing:... (92 hits) Unfortunately I do not recall the details of the collision, it was while prototyping the WMPI unittests before the pipeline_impl refactor. https://codereview.chromium.org/1641423002/diff/300001/media/base/mock_filters.h File media/base/mock_filters.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/base/mock_filter... media/base/mock_filters.h:72: // TODO(sandersd): These should probably have setters to. On 2016/02/26 02:30:11, wolenetz wrote: > nit: too Done. https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:891: if (resuming_ && playback_rate_ > 0 && !paused_) On 2016/02/26 02:54:24, DaleCurtis wrote: > See https://codereview.chromium.org/1739473003 -- I think you have some issues > here that I fixed in that CL. You've deleted this chunk of code, but by doing > that I think you're always signaling the delegates when in the suspended state, > which will hold the power save blocker. That can happen if JS issues a play() > call to a tag bestowed with the magical user gesture (or said restriction is > disabled). > > You can test with this page > http://storage.googleapis.com/dalecurtis-shared/bg_start.html Done. https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:864: // Ignore state changes until we've completed all outstanding operations. On 2016/02/26 02:30:11, wolenetz wrote: > What if the pipeline ends while suspending or resuming request to it haven't > reached it yet, but the pipelinecontroller knows it's being suspended or > resumed? Will we drop the "pipeline ended" notification on the floor now versus > prior to this change? Summary of offline discussion: This is okay for two reasons; (1) IsStable() does not include suspended, and (2) OnEnded must be delivered before OnSuspended. There is future work to make sure we don't resume if we were ended, but it depends on Dale's pending CL. https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.h:193: // TODO(hubbe): Make private. On 2016/02/26 02:54:24, DaleCurtis wrote: > Please explicitly use the words WMPI_CAST so that its easily found via search. Done. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... File media/filters/pipeline_controller.cc (right): https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.cc:51: On 2016/02/26 02:30:11, wolenetz wrote: > Let's further enforce the contract that, if chunk_demuxer is set, then it must > be the same demuxer as demuxer... e.g. DCHECK(!chunk_demuxer || (chunk_demuxer > == demuxer)). This was previously done inside one method in WMPI, now it's split > across two classes, so let's keep it clean :) > Perhaps this can be further cleaned-up in that follow-up CL by just using a > boolean to indicate if we're using MSE, or use two injected methods or something > for those specific non-Demuxer methods that ChunkDemuxer needs to be signalled > with by PipelineController in MSE case. Done. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:18: class ChunkDemuxer; On 2016/02/26 02:30:11, wolenetz wrote: > per chat, in follow-up CL, please find a way of testing the ChunkDemuxer-API > surface area exercised by PipelineController. The specialization of seek > behavior for ChunkDemuxer in combination with suspend/resume/pause/cast/etc. is > a risky surface area to keep untested for long. > Among several alternatives, one potentially simple one might be to identify > which methods (StartWaitingForSeek(...),CancelPendingSeek(...)) of ChunkDemuxer > that are not in Demuxer interface are exercised by PipelineController, and see > if they can be elevated to the Demuxer interface with default no-op > implementations there for non-ChunkDemuxers. Then, various for-testing methods > on PipelineController can be exercised by the unit tests to make it think it's > operating on a ChunkDemuxer, and verify that the StartWaitingForSeek/etc methods > occur when they should (perhaps by injecting them as for-test callbacks). Acknowledged. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:97: bool IsStable(); On 2016/02/26 02:30:11, wolenetz wrote: > aside: I like "stable state" concept here. Be wary, though, of potential > confusion with HTMLMediaElement/Blink "stable state" concept, which is totally > different :) Acknowledged. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:139: bool pending_seeked_ = false; On 2016/02/26 02:30:11, wolenetz wrote: > nit: s/pending_seeked_/pending_seek_completed_/ to help prevent confusion with > very similar |pending_seek_|. Renamed to include _cb.
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/320001
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/340001
lgtm % couple fixes once tests are passing. https://codereview.chromium.org/1641423002/diff/340001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/340001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1361: if (delegate_) You need to block these too on issupended.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % *tiny* nit https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:864: // Ignore state changes until we've completed all outstanding operations. On 2016/02/26 22:10:48, sandersd wrote: > On 2016/02/26 02:30:11, wolenetz wrote: > > What if the pipeline ends while suspending or resuming request to it haven't > > reached it yet, but the pipelinecontroller knows it's being suspended or > > resumed? Will we drop the "pipeline ended" notification on the floor now > versus > > prior to this change? > > Summary of offline discussion: This is okay for two reasons; (1) IsStable() does > not include suspended, and (2) OnEnded must be delivered before OnSuspended. > There is future work to make sure we don't resume if we were ended, but it > depends on Dale's pending CL. Acknowledged. https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/300001/media/filters/pipeline... media/filters/pipeline_controller.h:139: bool pending_seeked_ = false; On 2016/02/26 22:10:48, sandersd wrote: > On 2016/02/26 02:30:11, wolenetz wrote: > > nit: s/pending_seeked_/pending_seek_completed_/ to help prevent confusion with > > very similar |pending_seek_|. > > Renamed to include _cb. Acknowledged. https://codereview.chromium.org/1641423002/diff/340001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/340001/media/filters/pipeline... media/filters/pipeline_controller.h:102: // - Start() is precessed immediately while in the CREATED state. nit: precession? Fancy pipeline :)
https://codereview.chromium.org/1641423002/diff/340001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1641423002/diff/340001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1361: if (delegate_) On 2016/02/26 22:34:34, DaleCurtis wrote: > You need to block these too on issupended. Done. https://codereview.chromium.org/1641423002/diff/340001/media/filters/pipeline... File media/filters/pipeline_controller.h (right): https://codereview.chromium.org/1641423002/diff/340001/media/filters/pipeline... media/filters/pipeline_controller.h:102: // - Start() is precessed immediately while in the CREATED state. On 2016/02/26 23:21:06, wolenetz wrote: > nit: precession? Fancy pipeline :) Done.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1641423002/#ps380001 (title: "Typo")
The CQ bit was unchecked by sandersd@chromium.org
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
The CQ bit was unchecked by sandersd@chromium.org
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/500001
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/560001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/580001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/600001
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 sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/620001
PTAL again, as it's been a while since the LGTMs. dalecurtis@: In particular, make sure I didn't undo any of the fixes you landed.
Re-reviewed WMPI, lgtm
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 sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1641423002/#ps620001 (title: "Remove debugging code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/620001
Message was sent while issue was closed.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ==========
Message was sent while issue was closed.
Committed patchset #32 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. Committed: https://crrev.com/aafb73c6460349c508a49e6e6de21e774ec80b5c Cr-Commit-Position: refs/heads/master@{#379168} ==========
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/aafb73c6460349c508a49e6e6de21e774ec80b5c Cr-Commit-Position: refs/heads/master@{#379168}
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:620001) has been created in https://codereview.chromium.org/1759123003/ by yoichio@chromium.org. The reason for reverting is: This CL has made layout tests failed. See: https://bugs.chromium.org/p/chromium/issues/detail?id=591930.
Message was sent while issue was closed.
A revert of this CL (patchset #32 id:620001) has been created in https://codereview.chromium.org/1762963002/ by sigbjornf@opera.com. The reason for reverting is: Causes a pair of layout test failures on linux(dbg), https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg... It _might_ also be behind the MSan failures https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b... , but not locally confirmed..
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
Were (Blink) layout tests run for this CL? It seems like the bots analyzed that not to be needed & there's no explicit include of any tryserver.blink bots afaict.
Message was sent while issue was closed.
On 2016/03/04 07:56:25, yoichio wrote: > A revert of this CL (patchset #32 id:620001) has been created in > https://codereview.chromium.org/1759123003/ by mailto:yoichio@chromium.org. > > The reason for reverting is: This CL has made layout tests failed. See: > https://bugs.chromium.org/p/chromium/issues/detail?id=591930. Sorry, revert collision.
Message was sent while issue was closed.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. Committed: https://crrev.com/aafb73c6460349c508a49e6e6de21e774ec80b5c Cr-Commit-Position: refs/heads/master@{#379168} ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. Committed: https://crrev.com/aafb73c6460349c508a49e6e6de21e774ec80b5c Cr-Commit-Position: refs/heads/master@{#379168} ==========
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. Committed: https://crrev.com/aafb73c6460349c508a49e6e6de21e774ec80b5c Cr-Commit-Position: refs/heads/master@{#379168} ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ==========
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/640001
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 sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1641423002/#ps640001 (title: "Initialize |should_notify_time_changed_|.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641423002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641423002/640001
Message was sent while issue was closed.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ==========
Message was sent while issue was closed.
Committed patchset #33 (id:640001)
Message was sent while issue was closed.
Description was changed from ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. ========== to ========== Extract state management from WebMediaPlayerImpl. WMPI recieves operations at any time but is backed by Pipeline which can only perform one operation at a time. This CL creates a helper class (PipelineController) to translate between the two so that WMPI does not need to handle the full cross product of states. Committed: https://crrev.com/1c0bba0d8cbd1390822796b22bfa9bc353d4b5c8 Cr-Commit-Position: refs/heads/master@{#379386} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/1c0bba0d8cbd1390822796b22bfa9bc353d4b5c8 Cr-Commit-Position: refs/heads/master@{#379386} |
