Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
51613002:
Abort MSP::OnPrefetchDone() if just after MSP::Release(). Let seek and config change survive. (Closed)
|
Created:
7 years, 1 month ago by wolenetz Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAbort MSP::OnPrefetchDone() if just after MSP::Release(). Let seek and config change survive.
Prevents DCHECK'ing on unexpectedly missing pending prefetch done event
in MSP::OnPrefetchDone() since OnPrefetchDone() could already be posted
when Release() occurs.
Also allows pending seek or config change to survive Release().
More robust fix for correctly tracking player and stream state across MSP::Release() + Start() is tracked by http://crbug.com/306314.
Also, http://crbug.com/304234 tracks more robust buffering by browser player that may modify prefetch behavior.
R=qinmin@chromium.org,acolwell@chromium.org
BUG=302867, 306314, 304234
TEST=All media_unittests pass locally on Android with MediaCodecBridge available.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232601
Patch Set 1 #Patch Set 2 : remove an extra blank line #
Total comments: 8
Patch Set 3 : address Min's PS2 comments #Patch Set 4 : don't ProcessPendingEvents() if aborting OnPrefetchDone() #Patch Set 5 : allow pending seek or config change to survive player Release(), tests not fully done yet #Patch Set 6 : add more unit test coverage #
Total comments: 10
Patch Set 7 : address comments and nits from PS6 #
Total comments: 13
Patch Set 8 : Address comments and nits from PS7 #
Messages
Total messages: 25 (0 generated)
Please take a look @ PS2: qinmin@: OWNER review of all, acolwell@: review all too, please. Thanks!
On 2013/10/29 22:37:08, wolenetz wrote: > Please take a look @ PS2: > qinmin@: OWNER review of all, > acolwell@: review all too, please. > > Thanks! With verbose logging turned on, the sequencing of the player behavior for the new test is clear and similar to what is seen in bug 302867: [VERBOSE1:media_source_player.cc(353)] OnDemuxerDataAvailable(1) [VERBOSE1:media_decoder_job.cc(43)] OnDataReceived: 5 units [VERBOSE1:media_decoder_job.cc(270)] DecodeInternal [VERBOSE1:media_decoder_job.cc(158)] QueueInputBuffer [VERBOSE1:media_source_player.cc(799)] OnDecoderStarved [VERBOSE1:media_source_player.cc(906)] SetPendingEvent(PREFETCH_REQUEST) [VERBOSE1:media_source_player.cc(469)] ProcessPendingEvents : 0x8 [VERBOSE1:media_source_player.cc(477)] ProcessPendingEvents : An audio job is still decoding. [VERBOSE1:media_source_player.cc(543)] MediaDecoderCallback: 1, 3 [VERBOSE1:media_source_player.cc(469)] ProcessPendingEvents : 0x8 [VERBOSE1:media_source_player.cc(515)] ProcessPendingEvents : Handling PREFETCH_REQUEST_EVENT. [VERBOSE1:media_decoder_job.cc(70)] Prefetch : using previously received data [VERBOSE1:media_source_player.cc(906)] SetPendingEvent(PREFETCH_DONE) [VERBOSE1:media_source_player.cc(914)] ClearPendingEvent(PREFETCH_REQUEST) // Note the sequencing of the following is critical to repro of bug 302867: [VERBOSE1:media_source_player.cc(232)] Release [VERBOSE1:media_decoder_job.cc(139)] Release [VERBOSE1:media_source_player.cc(847)] OnPrefetchDone // This is where the code previously hit DCHECK. [VERBOSE1:media_source_player.cc(858)] OnPrefetchDone : aborting [VERBOSE1:media_source_player.cc(312)] OnDemuxerConfigsAvailable [VERBOSE1:media_source_player.cc(170)] Start [VERBOSE1:media_source_player.cc(284)] StartInternal
https://codereview.chromium.org/51613002/diff/80001/content/browser/media/and... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/80001/content/browser/media/and... content/browser/media/android/browser_media_player_manager.h:103: virtual void OnMediaDecoderCallback() OVERRIDE {} For test functions, normally use names like "OnMediaDecoderCallbackForTesting()" https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_player_manager.h:111: virtual void OnMediaDecoderCallback() = 0; use sth like OnMediaDecoderCallbackForTesting() https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_source_player.cc:861: ProcessPendingEvents(); Do we really need to process the pending events? if we have pending events, these pending events should arrive after release(). So they are already being handled by corresponding functions.
Thanks. PTAL @ PS3: https://codereview.chromium.org/51613002/diff/80001/content/browser/media/and... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/80001/content/browser/media/and... content/browser/media/android/browser_media_player_manager.h:103: virtual void OnMediaDecoderCallback() OVERRIDE {} On 2013/10/29 23:49:34, qinmin wrote: > For test functions, normally use names like "OnMediaDecoderCallbackForTesting()" Done. Note that there is now an associated presubmit warning for the call to this from production code (in media_source_player.cc:560): "You might be calling functions intended only for testing from production code. It is OK to ignore this warning if you know what you are doing, as the heuristics used to detect the situation are not perfect. The commit queue will not block on this warning." https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_player_manager.h:111: virtual void OnMediaDecoderCallback() = 0; On 2013/10/29 23:49:34, qinmin wrote: > use sth like OnMediaDecoderCallbackForTesting() Done. https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_source_player.cc:861: ProcessPendingEvents(); On 2013/10/29 23:49:34, qinmin wrote: > Do we really need to process the pending events? if we have pending events, > these pending events should arrive after release(). So they are already being > handled by corresponding functions. Suppose we had prefetch done pending when those other events arrived after release(). In that case, ProcessPendingEvents() would not have previously processed them due to pending prefetch done event. In reality, we probably don't do the right thing for those events (regardless of ProcessPendingEvents() here) until the overall bug 306314 is fixed. https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_source_player_unittest.cc:1507: EXPECT_FALSE(GetMediaDecoderJob(true)); This line was redundant with while(), above.
https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... media/base/android/media_source_player.cc:861: ProcessPendingEvents(); But release() will clear the pending_event_, so there is no PREFETCH_DONE event pending. Right? And if a prefetch done task is posted at that state, while a seek is performed right before the task gets executed, the seek will be executed immediately On 2013/10/30 00:06:45, wolenetz wrote: > On 2013/10/29 23:49:34, qinmin wrote: > > Do we really need to process the pending events? if we have pending events, > > these pending events should arrive after release(). So they are already being > > handled by corresponding functions. > > Suppose we had prefetch done pending when those other events arrived after > release(). In that case, ProcessPendingEvents() would not have previously > processed them due to pending prefetch done event. In reality, we probably don't > do the right thing for those events (regardless of ProcessPendingEvents() here) > until the overall bug 306314 is fixed.
On 2013/10/30 00:06:45, wolenetz wrote: > Thanks. PTAL @ PS3: > ... > https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... > media/base/android/media_source_player.cc:861: ProcessPendingEvents(); > On 2013/10/29 23:49:34, qinmin wrote: > > Do we really need to process the pending events? if we have pending events, > > these pending events should arrive after release(). So they are already being > > handled by corresponding functions. > > Suppose we had prefetch done pending when those other events arrived after > release(). In that case, ProcessPendingEvents() would not have previously > processed them due to pending prefetch done event. In reality, we probably don't > do the right thing for those events (regardless of ProcessPendingEvents() here) > until the overall bug 306314 is fixed. > > https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/51613002/diff/80001/media/base/android/media_... > media/base/android/media_source_player_unittest.cc:1507: > EXPECT_FALSE(GetMediaDecoderJob(true)); > This line was redundant with while(), above. s/we had prefetch done pending/we had OnPrefetchDone() posted/ I haven't yet found a good example though. My tentative example is flawed: 1. SeekTo requested while decoding in progress. ProcessPendingEvents() doesn't process it yet due to still decoding. 2. Trivial prefetch triggered due to starvation. ProcessPending, posts OnPrefetchDone() to loop. Hmm. This actually won't happen because the Seek will begin first. 3. Release() runs. I'm not clear of a strong case for or against ProcessPendingEvents() when aborting OnPrefetchDone().
PTAL @ PS4. It removes the ProcessPendingEvents() call from the case where OnPrefetchDone() is aborting. I don't think we can DCHECK(pending_event_ == NO_EVENT_PENDING) either, because that SeekTo prior to Prefetch race with Release would have initiated the seek, but the OnDemuxerSeekDone() is very likely not going to happen before OnPrefetchDone(), so the seek event will still be pending.
On 2013/10/30 00:23:58, wolenetz wrote: > PTAL @ PS4. It removes the ProcessPendingEvents() call from the case where > OnPrefetchDone() is aborting. I don't think we can DCHECK(pending_event_ == > NO_EVENT_PENDING) either, because that SeekTo prior to Prefetch race with > Release would have initiated the seek, but the OnDemuxerSeekDone() is very > likely not going to happen before OnPrefetchDone(), so the seek event will still > be pending. Hmm. That example is also flawed. Seek will have been initiated but pending state lost due to Release() clearing it. And so eventual OnDemuxerSeekDone() might have similar DCHECK issue (bug 306314). I don't yet have an example showing we cannot DCHECK_EQ(pending_event_, NO_EVENT_PENDING). wdyt?
On 2013/10/30 00:29:43, wolenetz wrote: > On 2013/10/30 00:23:58, wolenetz wrote: > > PTAL @ PS4. It removes the ProcessPendingEvents() call from the case where > > OnPrefetchDone() is aborting. I don't think we can DCHECK(pending_event_ == > > NO_EVENT_PENDING) either, because that SeekTo prior to Prefetch race with > > Release would have initiated the seek, but the OnDemuxerSeekDone() is very > > likely not going to happen before OnPrefetchDone(), so the seek event will > still > > be pending. > > Hmm. That example is also flawed. > Seek will have been initiated but pending state lost due to Release() clearing > it. And so eventual OnDemuxerSeekDone() might have similar DCHECK issue (bug > 306314). > I don't yet have an example showing we cannot DCHECK_EQ(pending_event_, > NO_EVENT_PENDING). wdyt? If Seek Arrives before release(), that's a separate issue. We can remove the DCHECKs or don't clean up pending seeks in release(). Since seek can arrive after release(), and right before OnPrefetchDone（）, so OnPrefetchDone() cannot DCHECK no_event_pending and cannot call processPendingEvents.
On 2013/10/30 01:54:24, qinmin wrote: > On 2013/10/30 00:29:43, wolenetz wrote: > > On 2013/10/30 00:23:58, wolenetz wrote: > > > PTAL @ PS4. It removes the ProcessPendingEvents() call from the case where > > > OnPrefetchDone() is aborting. I don't think we can DCHECK(pending_event_ == > > > NO_EVENT_PENDING) either, because that SeekTo prior to Prefetch race with > > > Release would have initiated the seek, but the OnDemuxerSeekDone() is very > > > likely not going to happen before OnPrefetchDone(), so the seek event will > > still > > > be pending. > > > > Hmm. That example is also flawed. > > Seek will have been initiated but pending state lost due to Release() clearing > > it. And so eventual OnDemuxerSeekDone() might have similar DCHECK issue (bug > > 306314). > > I don't yet have an example showing we cannot DCHECK_EQ(pending_event_, > > NO_EVENT_PENDING). wdyt? > > If Seek Arrives before release(), that's a separate issue. We can remove the > DCHECKs or don't clean up pending seeks in release(). > Since seek can arrive after release(), and right before OnPrefetchDone（）, so > OnPrefetchDone() cannot DCHECK no_event_pending and cannot call > processPendingEvents. Ok. PS4 does not DCHECK(NO_EVENT_PENDING) in the OnPrefetchDone() aborting case. I'm considering updating this CL with change to Release() to not clear pending Seek event, with associated unit tests. Regarding the OnPrefetchDone() changes so far (PS4), do you have any comments? Thanks, Matt
PTAL @ PS5 - Tests are incomplete, but the production code is there to allow SEEK_EVENT_PENDING and CONFIG_CHANGE_EVENT_PENDING to survive beyond player Release(). This seems a better approach than clearing them in Release(), since they could already be in progress, expected by web app/user or even demuxer. This still doesn't fix the job's reset causing received data to be dropped, nor double-data-request potential (tracked by bug 306314 & 304234).
PS6 is ready for review. PTAL. Thanks!
https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player.cc:243: if (IsEventPending(SEEK_EVENT_PENDING) || No need for this if statement, processPendingEvents will nothing if there are no pending seek or config change events. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1754: EXPECT_FALSE(GetMediaDecoderJob(false)); lots of the testing code are testing the same initial sequence, can we remove them?
In general I think the tests are verifying too much of MSP's internal state. I think they should just focus on the external interactions that trigger certain states and verify the observable outcomes on objects MSP is interacting with. I think this would reduce clutter in the tests and make it easier to see what external stimuli are important for MSP to handle. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_player_manager.h:111: virtual void OnMediaDecoderCallbackForTesting() = 0; This does not seem right. Why don't you just put a set_decode_callback_for_testing() method on MSP? Putting this on the manager seems like a bad idea. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player.cc:249: if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING)) nit: How about the following instead: // Clear all pending events except seeks and config changes. pending_event_ &= (SEEK_EVENT_PENDING | CONFIG_CHANGE_EVENT_PENDING); https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player_unittest.cc:85: virtual void OnMediaDecoderCallbackForTesting() OVERRIDE { This should be in MSP not here.
These comments are very helpful. Thank you, both. Regarding, "In general I think the tests are verifying too much of MSP's internal state...", I removed almost all inspection by tests of player's pending_event_ state. The only place where I kept it was to ensure the specific original repro scenario in bug 302867 is met (in ReleaseWithPendingPrefetchDoneVerification(), used by the ReleaseWithOnPrefetchDoneAlreadyPosted test). https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_player_manager.h (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_player_manager.h:111: virtual void OnMediaDecoderCallbackForTesting() = 0; On 2013/10/30 23:29:53, acolwell wrote: > This does not seem right. Why don't you just put a > set_decode_callback_for_testing() method on MSP? Putting this on the manager > seems like a bad idea. Thanks for catching this. Putting this in MSP simplifies the test code a little, too (one less method to mock), and keeps the callback specific to the intended player. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player.cc:243: if (IsEventPending(SEEK_EVENT_PENDING) || On 2013/10/30 23:07:48, qinmin wrote: > No need for this if statement, processPendingEvents will nothing if there are no > pending seek or config change events. Excellent point, thank you. Done. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player.cc:249: if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING)) On 2013/10/30 23:29:53, acolwell wrote: > nit: How about the following instead: > > // Clear all pending events except seeks and config changes. > pending_event_ &= (SEEK_EVENT_PENDING | CONFIG_CHANGE_EVENT_PENDING); Done. I was trying to keep the raw pending_event_ modifications to Set/Clear/Is* methods, in case the underlying implementation changed from a bitmap. This is simpler though. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player_unittest.cc:85: virtual void OnMediaDecoderCallbackForTesting() OVERRIDE { On 2013/10/30 23:29:53, acolwell wrote: > This should be in MSP not here. Done. https://codereview.chromium.org/51613002/diff/380001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1754: EXPECT_FALSE(GetMediaDecoderJob(false)); On 2013/10/30 23:07:48, qinmin wrote: > lots of the testing code are testing the same initial sequence, can we remove > them? Yes, indeed. I've coalesced many of the duplicate verifications and setups. I also removed my "// BIG TODO" comment ! :)
lgtm % nit https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1462: // Upon the next successful decode callback, post a task to Release() nit: rephrase this. either "release |player_|" or "call Release() on the |player_|".
in general this looks good, but I still have a few comments related to the tests. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player.h:157: // Test-only method to setup hook for the completion of the next decode cycle. nit: You should add that this callback state is cleared when it gets run. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:188: player_.set_decode_callback_for_testing(test_decode_cb); nit: This is only called in one location. Please just inline at the call site. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1465: SetTestDecodeCallback(media::BindToLoop( Do you really need this callback? Can't you just check for the pending prefetch done in the loop below and trigger the release there? Since you are posting a task with the callback, it seems like this should be possible. If not, then does changing the number of AUs you make available above allow you to have better control over the prefetch so you don't need the callback? https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1598: // removal of browser seeks, consider retaining a regular seek version of this This is confusing to me. Does this mean a new test needs to be created when browser seek functionality has been removed? If the regular seek & browser seek behavior should be the same, shouldn't we just test the regular seek version so that we don't have to add non-browser seek test cases to maintain coverage when we remove that functionality? https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1635: // test. ditto
I believe PS8 addresses all outstanding comments and nits. acolwell@, PTAL. qinmin@, no production code was changed since PS7 except for your comment nit, so I believe no further lgt* from you is necessary. Thanks! https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player.h:157: // Test-only method to setup hook for the completion of the next decode cycle. On 2013/10/31 17:54:00, acolwell wrote: > nit: You should add that this callback state is cleared when it gets run. Done. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:188: player_.set_decode_callback_for_testing(test_decode_cb); On 2013/10/31 17:54:00, acolwell wrote: > nit: This is only called in one location. Please just inline at the call site. I did this to keep MSP::set_decode_callback_for_testing() private. The TEST_F bodies do not have access to private MSP members. From our chat, I will keep this test decode callback helper and move the binding setup into it from the only test that uses it, below. I'll also add warnings here and in MSP.h to help prevent usage creep of the test-only MSP method. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1462: // Upon the next successful decode callback, post a task to Release() On 2013/10/31 04:12:27, qinmin wrote: > nit: rephrase this. either "release |player_|" or "call Release() on the > |player_|". Done. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1462: // Upon the next successful decode callback, post a task to Release() On 2013/10/31 04:12:27, qinmin wrote: > nit: rephrase this. either "release |player_|" or "call Release() on the > |player_|". Done. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1465: SetTestDecodeCallback(media::BindToLoop( On 2013/10/31 17:54:00, acolwell wrote: > Do you really need this callback? Can't you just check for the pending prefetch > done in the loop below and trigger the release there? Since you are posting a > task with the callback, it seems like this should be possible. If not, then does > changing the number of AUs you make available above allow you to have better > control over the prefetch so you don't need the callback? The problem is that RunUntilIdle() runs too much, and test needs to replicate the exact scenario from bug 302867 where Release() is posted during the decode cycle just after entering |PREFETCH_REQUEST_EVENT_PENDING| and just before the resulting post-task of OnPrefetchDone(). If we tried to just PostTask(MSP::Release()) immediately after TriggerPlayerStarvation(), there is no guarantee that decoding has just completed and already posted a task to MDJ::OnDecodeCompleted()->MSP::MediaDecoderCallback()->ProcessPendingEvents(do the trivial prefetch+Posttask(OnPrefetchDone barrier). For clarity, I think this bug is hit when: 1. Decoder job is decoding and has at least one more access unit beyond the one currently being decoded. 2. Starvation callback is fired. Sets PREFETCH_REQUEST_EVENT_PENDING, but ProcessPendingEvents() doesn't yet process it because job is still decoding. 3. MDJ::OnDecodeCompleted() task is posted to UI loop from decoder loop. 4. Before the posted task from #3 is run, some other task that calls MSP::Release() is posted to UI loop. 5. Tasks from 3 and 4 execute on UI loop, resulting in following sequence: MSP::MediaDecoderCallback(), then MSP::Release(), then MSP::OnPrefetchDone(). Per comment, above, I'll relocate the binding of the specific test callback to a helper method and add comment that this binding is specifically for replicating this fragile test scenario that is due to MDJ::Prefetch() sometimes post-tasking the prefetch done barrier. Ideally, this will all get ripped out (including the MSP test-only decode hook interface) once prefetch is reworked along with buffer reworking in http://crbug.com/304234. If MSP::OnPrefetchDone() processing is reentrant-safe, we could simplify in short-term by not PostTask'ing to the prefetch done callback in MDJ::Prefetch(), and instead serially Run() it. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1598: // removal of browser seeks, consider retaining a regular seek version of this On 2013/10/31 17:54:00, acolwell wrote: > This is confusing to me. Does this mean a new test needs to be created when > browser seek functionality has been removed? If the regular seek & browser seek > behavior should be the same, shouldn't we just test the regular seek version so > that we don't have to add non-browser seek test cases to maintain coverage when > we remove that functionality? Oops! My 'Note' comments in these two new BrowserSeek tests were internally inconsistent. I've removed the 'Notes' and added analogous regular seek cases, too. BrowserSeek versions are also kept. https://codereview.chromium.org/51613002/diff/530001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1635: // test. On 2013/10/31 17:54:00, acolwell wrote: > ditto Done, per above. Thanks!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/51613002/730001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/51613002/730001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/51613002/730001
Message was sent while issue was closed.
Change committed as 232601 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
