Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
79283006:
Let only seeks reset Android MSE stream playback completion (Closed)
|
Created:
7 years, 1 month ago by wolenetz Modified:
7 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLet only seeks reset Android MSE stream playback completion
For audio/video MediaSource playback on Android, if one or more of the streams
finishes playback, preserve that state across config changes and player
Release()+Start(). Only reset this state in MSP::OnDemuxerSeekDone().
Includes work-arounds for undefined MediaCodec behavior on attempted
decode after previous output EOS decode without intervening flush.
Includes new unit tests and some test cleanup.
BUG=269784
R=qinmin@chromium.org,acolwell@chromium.org
TEST=All MSP unit tests pass on Android with MediaCodecBridge available. mediasource-config-change-mp4-av-framesize layout test passes.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240500
Patch Set 1 #Patch Set 2 : fix lint errors #
Total comments: 2
Patch Set 3 : Rebase, LOG(INFO)->VLOG(0), and spot-check in tests that decoder doesn't resume after EOS processed #
Total comments: 13
Patch Set 4 : Rebased and addresses PS3 comments #
Total comments: 2
Patch Set 5 : Ignore output_eos decode result if seek is pending #
Total comments: 23
Patch Set 6 : Rebased and addressed comments+nits from PS5 #
Total comments: 20
Patch Set 7 : Rebased, addressed ps6 comments. includes some test cleanup #
Total comments: 2
Patch Set 8 : Address qinmin's nits from PS7 #
Messages
Total messages: 27 (0 generated)
PTAL @ patch set 2. qinmin@: OWNERS review acolwell@: fyi Note, neither of the two new unit tests actually failed for me without the rest of this patch. My hypothesis is this is MediaCodec having undefined behavior for later output dequeue attempts once output-EOS has been dequeued. In the layout test, this caused persistent dequeue-output-again-later instead of what I saw from the two new unit tests (output-EOS). Over 40 repetitions of the layout test passed (vs a timeout rate without this patch of over 50%).
https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... media/base/android/media_source_player_unittest.cc:1075: GetMediaDecoderJob(false)->is_decoding()) { if video is reaching EOS, shouldn't video decoder job no longer be in a decoding state again? GetMediaDecoderJob(false)->is_decoding() feels very misleading
On 2013/11/21 03:25:44, qinmin wrote: > https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... > media/base/android/media_source_player_unittest.cc:1075: > GetMediaDecoderJob(false)->is_decoding()) { > if video is reaching EOS, shouldn't video decoder job no longer be in a decoding > state again? > GetMediaDecoderJob(false)->is_decoding() feels very misleading Great point, thanks. Fixing this may also allow me to get at least this new unit test to fail without the rest of the patch applied. I'll work on this for inclusion in the next patch set.
PTAL @ patch set 3. Though both of the new unit tests conceivably could fail without the rest of the patch, only NoPrefetchForAlreadyFinishedStreamOnStarvation does in practice because the checks for post-EOS decode resumption are only performed intermittently. RunUntilIdle() runs too much/re-decode of EOS is too fast. With the rest of this patch, all MSPUT pass (and none hit the new DCHECKs in MSP). https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... media/base/android/media_source_player_unittest.cc:1075: GetMediaDecoderJob(false)->is_decoding()) { On 2013/11/21 03:25:44, qinmin wrote: > if video is reaching EOS, shouldn't video decoder job no longer be in a decoding > state again? > GetMediaDecoderJob(false)->is_decoding() feels very misleading Done.
On 2013/11/23 00:10:36, wolenetz wrote: > PTAL @ patch set 3. > Though both of the new unit tests conceivably could fail without the rest of the > patch, only NoPrefetchForAlreadyFinishedStreamOnStarvation does in practice > because the checks for post-EOS decode resumption are only performed > intermittently. RunUntilIdle() runs too much/re-decode of EOS is too fast. With > the rest of this patch, all MSPUT pass (and none hit the new DCHECKs in MSP). > > https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/79283006/diff/40001/media/base/android/media_... > media/base/android/media_source_player_unittest.cc:1075: > GetMediaDecoderJob(false)->is_decoding()) { > On 2013/11/21 03:25:44, qinmin wrote: > > if video is reaching EOS, shouldn't video decoder job no longer be in a > decoding > > state again? > > GetMediaDecoderJob(false)->is_decoding() feels very misleading > > Done. Ping (patch set 3 is pending review). Thank you.
https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:534: int count = ((audio_decoder_job_ && !audio_finished_) ? 1 : 0) + I think the audio_decoder_ && !audio_finished_ condition should be moved into a helper function named something like CanDecodeAudio() or AudioNotFinished(). Something similar should be done for video as well. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:717: if ((!HasAudio() || audio_finished_) && (!HasVideo() || video_finished_)) { This makes me wonder whether audio_decoder_job_ && !audio_finished_ is the right condition for the "audio isn't finished" condition. 1. Should HasAudio() be factored into the other conditions you've added? 2. Could this be changed to audio_decoder_job_ && !audio_finished_? 3. Could this become (!AudioNotFinished() && !VideoNotFinished()) or (AudioFinished() && VideoFinished()) and reverse the sense of the checks elsewhere. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1042: // audio decoder job resume decoding (except after a seek). nit: The "(except after a seek)" comment here seems out of place since we don't actually issue a seek in this test. I think it should be removed.? https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1086: // above, should the video decoder job resume decoding (except after a seek). ditto https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1100: Are audio-only and video-only playback_completed cases covered by other tests?
On 2013/11/25 21:43:59, acolwell wrote: > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player.cc:534: int count = ((audio_decoder_job_ > && !audio_finished_) ? 1 : 0) + > I think the audio_decoder_ && !audio_finished_ condition should be moved into a > helper function named something like CanDecodeAudio() or AudioNotFinished(). > Something similar should be done for video as well. > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player.cc:717: if ((!HasAudio() || > audio_finished_) && (!HasVideo() || video_finished_)) { > This makes me wonder whether audio_decoder_job_ && !audio_finished_ is the right > condition for the "audio isn't finished" condition. > > 1. Should HasAudio() be factored into the other conditions you've added? > 2. Could this be changed to audio_decoder_job_ && !audio_finished_? > 3. Could this become (!AudioNotFinished() && !VideoNotFinished()) or > (AudioFinished() && VideoFinished()) and reverse the sense of the checks > elsewhere. > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1042: // audio decoder job > resume decoding (except after a seek). > nit: The "(except after a seek)" comment here seems out of place since we don't > actually issue a seek in this test. I think it should be removed.? > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1086: // above, should the > video decoder job resume decoding (except after a seek). > ditto > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1100: > Are audio-only and video-only playback_completed cases covered by other tests? Thanks acolwell@. I'll consider adding DCHECKs to clarify current assumption that prefetch event processing can only be done if there are already a/v_decoder jobs corresponding to HasA/V(). Next patch set iteration may not occur until next week; no need for further review until then.
https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:601: PlaybackCompleted(is_audio); PlaybackCommpleted is executed after ProcessPendingEvents(). If there is a PREFETCH_REQUEST_EVENT, then that event could cause MSP to call StartInternal() without setting audio_finished_/video_finished_. And causing the infinite DEQUEUE_OUTPUT_AGAIN_LATER issue. We should reverse the order of this if statement and the if statement above. And change PlaybackCompleted() to check and process all the pending events.
On 2013/11/26 23:49:47, qinmin wrote: > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... > media/base/android/media_source_player.cc:601: PlaybackCompleted(is_audio); > PlaybackCommpleted is executed after ProcessPendingEvents(). > If there is a PREFETCH_REQUEST_EVENT, then that event could cause MSP to call > StartInternal() without setting audio_finished_/video_finished_. And causing the > infinite DEQUEUE_OUTPUT_AGAIN_LATER issue. > > We should reverse the order of this if statement and the if statement above. And > change PlaybackCompleted() to check and process all the pending events. Thanks qinmin@. I'll also consider including a unit test for this scenario, too, in the next patch set (not yet ready or uploaded, so this isn't a review request...)
https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:601: PlaybackCompleted(is_audio); Pending seek is the reason processPendingEvents should have higher priority than EOS. However, output_EOS should have higher priority than PREFETCH_REQUEST. Another way to tackle this issue is that we can stop the starvation callback if input EOS is already reached. In that case, we should not have PREFETCH_REQUEST pending when we got output_EOS. If we do this, then we don't need the reorder the if statement. On 2013/11/26 23:49:47, qinmin wrote: > PlaybackCommpleted is executed after ProcessPendingEvents(). > If there is a PREFETCH_REQUEST_EVENT, then that event could cause MSP to call > StartInternal() without setting audio_finished_/video_finished_. And causing the > infinite DEQUEUE_OUTPUT_AGAIN_LATER issue. > > We should reverse the order of this if statement and the if statement above. And > change PlaybackCompleted() to check and process all the pending events.
PTAL @ PS4. qinmin, especially help me understand if this version that follows your original recommendation of swapping the conditions in MSP::MediaDecoderCallback() is correct. Note also that I hit and filed http://crbug.com/325528 during verification, and have adjusted the test expectation for now to not fail due to that bug. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:534: int count = ((audio_decoder_job_ && !audio_finished_) ? 1 : 0) + On 2013/11/25 21:43:59, acolwell wrote: > I think the audio_decoder_ && !audio_finished_ condition should be moved into a > helper function named something like CanDecodeAudio() or AudioNotFinished(). > Something similar should be done for video as well. Done, with ternary swapped, as AudioFinishedOrNoAudio() + similar for video. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:601: PlaybackCompleted(is_audio); On 2013/11/26 23:49:47, qinmin wrote: > PlaybackCommpleted is executed after ProcessPendingEvents(). > If there is a PREFETCH_REQUEST_EVENT, then that event could cause MSP to call > StartInternal() without setting audio_finished_/video_finished_. And causing the > infinite DEQUEUE_OUTPUT_AGAIN_LATER issue. > > We should reverse the order of this if statement and the if statement above. And > change PlaybackCompleted() to check and process all the pending events. Done per the original comment, not the one from 12/3. One of the new tests I added fails though: VA_PlaybackCompletionAcrossConfigChange's OnPrefetchDone()'s DecodeMoreAudio() sees audio kConfigChanged, sets CONFIG_CHANGE_EVENT_PENDING, and then OnPrefetchDone()'s DecodeMoreVideo() decodes EOS while CONFIG_CHANGE_EVENT is still pending, leading to PlaybackCompleted() calling ProcessPendingEvents() and requesting a second time the demuxer configs. Per our chat, I've filed http://crbug.com/325528 and added reference to this in the unit test that exposes this bug (and updated the unit test to expect this redundant config change request meanwhile). https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player.cc:717: if ((!HasAudio() || audio_finished_) && (!HasVideo() || video_finished_)) { On 2013/11/25 21:43:59, acolwell wrote: > This makes me wonder whether audio_decoder_job_ && !audio_finished_ is the right > condition for the "audio isn't finished" condition. > > 1. Should HasAudio() be factored into the other conditions you've added? > 2. Could this be changed to audio_decoder_job_ && !audio_finished_? > 3. Could this become (!AudioNotFinished() && !VideoNotFinished()) or > (AudioFinished() && VideoFinished()) and reverse the sense of the checks > elsewhere. Done, following the latter option in 3, with a little extra distinction in name versus the {audio,video}_finished_ members, like {Audio,Video}FinishedOrNo{Audio,Video}(). Has{Audio,Video}() is now factored into those other conditions. I added DCHECKs in ProcessPendingEvents() for PREFETCH_REQUEST_EVENT_PENDING processing and in OnPrefetchDone() to clarify that !AudioFinishedOrNoAudio() implies audio_decoder_job_ must exist when handling that event/call (plus similar for video). Note that the current existence or lack of the decoder job for a finished stream does not change whether or not that stream is finished. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1042: // audio decoder job resume decoding (except after a seek). On 2013/11/25 21:43:59, acolwell wrote: > nit: The "(except after a seek)" comment here seems out of place since we don't > actually issue a seek in this test. I think it should be removed.? Done. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1086: // above, should the video decoder job resume decoding (except after a seek). On 2013/11/25 21:43:59, acolwell wrote: > ditto Done. https://codereview.chromium.org/79283006/diff/110001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1100: On 2013/11/25 21:43:59, acolwell wrote: > Are audio-only and video-only playback_completed cases covered by other tests? Existing tests had partial coverage: 1) ReplayInputAfterEOS (renamed to SecondVideoDataIsEOSAndSeekPlusStartResumesPlay and added playback_completed checks and refactored to use SeekPlayer()): video-only EOS completes playback + seek+start resumes play. I also dropped the test that was already a strict subset of this, called "NoRequestForDataAfterInputEOS" 2) First(Audio)DataIsEOS(AndSeekPlusStartResumesPlay): parts in parentheses added to this audio-only test 3) First(Audio)DataAfterSeekIsEOS: audio-only test I've also added cases: 1) FirstVideoDataAfterSeekIsEOS: video-only test 2) One case each for {audio,video}-only to verify edge-case behavior when starvation occurs during EOS decode. The A+V test already exists as (AV_)NoPrefetchForFinishedVideoOnAudioStarvation. (StarvationDuring{Audio,Video}EOSDecode) 3) An alternative case with A/V streams swapped, similar to PlaybackCompletionAcrossConfigChange 4) Added "SeekPlusStartResumesPlay" verification to end of {AV_,VA_}PlaybackCompletionAcrossConfigChange tests. Note that a helper to consolidate the very common code across {AV_,VA_}PlaybackCompletionAcrossConfigChange is not yet included, because one of the two tests currently exposes another bug (http://crbug.com/325528) and is adjusted to not fail until that bug is fixed.
https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... media/base/android/media_source_player.cc:727: if (pending_event_ != NO_EVENT_PENDING) i think you should process the pending events first. For example, playing_ may not be changed if the pending event is seek. That's why i think my other comments is better: rather than switch the order of complete event and pending event, just do a check to see if the stream is already reaching the end before sending out prefetch request.
On 2013/12/04 00:20:16, qinmin wrote: > https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... > media/base/android/media_source_player.cc:727: if (pending_event_ != > NO_EVENT_PENDING) > i think you should process the pending events first. > For example, playing_ may not be changed if the pending event is seek. > > That's why i think my other comments is better: rather than switch the order of > complete event and pending event, just do a check to see if the stream is > already reaching the end before sending out prefetch request. That makes sense. I think the other bug (http://crbug.com/325528) is orthogonal to this choice: I expect it would have pre-existed this CL and will probably exist after. I'll share another patch set once I work through the details and any other comments that may arrive in the interim. Thanks!
Please take a look at patch set 5. Thank you. https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... media/base/android/media_source_player.cc:727: if (pending_event_ != NO_EVENT_PENDING) On 2013/12/04 00:20:16, qinmin wrote: > i think you should process the pending events first. > For example, playing_ may not be changed if the pending event is seek. > > That's why i think my other comments is better: rather than switch the order of > complete event and pending event, just do a check to see if the stream is > already reaching the end before sending out prefetch request. From our chats/analyses, MediaDecoderCallback() can protect against completing playback if there was a seek that became pending during EOS decode by checking for that exact condition. All other pending events are unaffected by decode of EOS triggering playback completion (except PREFETCH_DONE_EVENT_PENDING, which is illegal during decode). We need to retain PREFETCH_REQUEST processing for any remaining unfinished stream, even if one of the potentially multiple streams decodes EOS. Patch set 5 includes these adjustments and associated new unit tests. We also discussed adding defense in depth by having MDJ reply with output_eos if it has had no intervening flush since original decoding output_eos. I've added a DCHECK+elevated output_eos_encountered to a member instead of hiding what would be an incorrect usage of MDJ.
On 2013/12/05 01:03:28, wolenetz wrote: > Please take a look at patch set 5. Thank you. > > https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/79283006/diff/160001/media/base/android/media... > media/base/android/media_source_player.cc:727: if (pending_event_ != > NO_EVENT_PENDING) > On 2013/12/04 00:20:16, qinmin wrote: > > i think you should process the pending events first. > > For example, playing_ may not be changed if the pending event is seek. > > > > That's why i think my other comments is better: rather than switch the order > of > > complete event and pending event, just do a check to see if the stream is > > already reaching the end before sending out prefetch request. > > From our chats/analyses, MediaDecoderCallback() can protect against completing > playback if there was a seek that became pending during EOS decode by checking > for that exact condition. All other pending events are unaffected by decode of > EOS triggering playback completion (except PREFETCH_DONE_EVENT_PENDING, which is > illegal during decode). We need to retain PREFETCH_REQUEST processing for any > remaining unfinished stream, even if one of the potentially multiple streams > decodes EOS. Patch set 5 includes these adjustments and associated new unit > tests. > > We also discussed adding defense in depth by having MDJ reply with output_eos if > it has had no intervening flush since original decoding output_eos. I've added a > DCHECK+elevated output_eos_encountered to a member instead of hiding what would > be an incorrect usage of MDJ. Ping, review patch set 5 please :)
https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:615: PlaybackCompleted(is_audio); // Includes ProcessPendingEvents() if needed. nit: Why don't you just let this fall through to the statement below? Then you don't need this comment or the call inside PlaybackCompleted(). This appears to be the only callsite so I think it is safe. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:764: bool MediaSourcePlayer::AudioFinishedOrNoAudio() { nit: I think the OrNoAudio() just confuses things at the call sites. How about just always initalizing 'audio_finished_ = !HasAudio()' everywhere you 'audio_finished_ = false;' You could put this in a ResetFinishedFlags() helper so you don't have to duplicate the code & explanitory comments. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:768: bool MediaSourcePlayer::VideoFinishedOrNoVideo() { ditto https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:954: DCHECK(audio_decoder_job_); nit: This seems redundant since DecodeMoreAudio() will crash if this isn't true. I'd rather have it crash there then leak preconditions to this callsite. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:959: DCHECK(video_decoder_job_); ditto https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:567: EXPECT_TRUE(have_audio || have_video); These should be DCHECKs since these are verifying preconditions. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:745: SeekPlayer(true, base::TimeDelta(), true, 1); nit: add and overload or default values so this type of change isn't needed here and below. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1123: EXPECT_EQ(5, demuxer_->num_data_requests()); nit: Do we really care about how many data requests there are? These seem to just clutter these new tests. It seems like the verification of these counts should be done in simple focused tests and left out of all other tests. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1142: while (GetMediaDecoderJob(true)->is_decoding() || nit: It seems like loops like this should just be put in some sort of WaitForDecodeDone() helper that allows you to control which streams you are waiting for.
lgtm on addressing all acolwell's comments, some nits https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.h:122: // Functions that check whether audio/video decode has reached end of output s/decode/stream/? https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:989: TEST_F(MediaSourcePlayerTest, SecondVideoDataIsEOSAndSeekPlusStartResumesPlay) { the test name is hard to understand. What about SecondVideoAccessUnitIsEOSAndResumePlayAfterSeek
Please take a look at patch set 6. Thanks! https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:615: PlaybackCompleted(is_audio); // Includes ProcessPendingEvents() if needed. On 2013/12/06 18:28:30, acolwell wrote: > nit: Why don't you just let this fall through to the statement below? Then you > don't need this comment or the call inside PlaybackCompleted(). This appears to > be the only callsite so I think it is safe. A little tweaking is also required to ensure that we do not proceed with remainder of this method beyond PlaybackCompleted() + conditional ProcessPendingEvents() after detecting output_eos here. Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:764: bool MediaSourcePlayer::AudioFinishedOrNoAudio() { On 2013/12/06 18:28:30, acolwell wrote: > nit: I think the OrNoAudio() just confuses things at the call sites. How about > just always initalizing 'audio_finished_ = !HasAudio()' everywhere you > 'audio_finished_ = false;' > > You could put this in a ResetFinishedFlags() helper so you don't have to > duplicate the code & explanitory comments. This makes sense with the current *_finished_ flags' naming, though additional tweaking of OnDemuxerConfigsAvailable() would be necessary: Has{Audio,Video}() potentially change across configuration change, from initial construction to changing the {audio, video} codec from unknown. An alternative approach is to rename *_finished_ to reached_*_eos_, and *FinishedOrNoVideo() to just *Finished(), without altering anything in the config change handling (since 1) eos cannot be reached for an unknown codec, and 2) everywhere we want to know if a stream is "finished", if the stream is unconfigured, we expect it to be finished). I've chosen the latter (renaming) approach. Outside of constructor initialization, these reached_*_eos_ members are only set to false in OnDemuxerSeekDone(), so I have not added a reset eos helper. Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:768: bool MediaSourcePlayer::VideoFinishedOrNoVideo() { On 2013/12/06 18:28:30, acolwell wrote: > ditto Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:954: DCHECK(audio_decoder_job_); On 2013/12/06 18:28:30, acolwell wrote: > nit: This seems redundant since DecodeMoreAudio() will crash if this isn't true. > I'd rather have it crash there then leak preconditions to this callsite. Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.cc:959: DCHECK(video_decoder_job_); On 2013/12/06 18:28:30, acolwell wrote: > ditto Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player.h (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player.h:122: // Functions that check whether audio/video decode has reached end of output On 2013/12/06 20:34:47, qinmin wrote: > s/decode/stream/? Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:567: EXPECT_TRUE(have_audio || have_video); On 2013/12/06 18:28:30, acolwell wrote: > These should be DCHECKs since these are verifying preconditions. Done. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:745: SeekPlayer(true, base::TimeDelta(), true, 1); On 2013/12/06 18:28:30, acolwell wrote: > nit: add and overload or default values so this type of change isn't needed here > and below. Done. Note http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... disallows usage of default argument values for this case, so I used an overload. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:989: TEST_F(MediaSourcePlayerTest, SecondVideoDataIsEOSAndSeekPlusStartResumesPlay) { On 2013/12/06 20:34:47, qinmin wrote: > the test name is hard to understand. What about > SecondVideoAccessUnitIsEOSAndResumePlayAfterSeek Done, using {A,V,AV,VA} prefixes across these new/changed tests for consistency and line length. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1123: EXPECT_EQ(5, demuxer_->num_data_requests()); On 2013/12/06 18:28:30, acolwell wrote: > nit: Do we really care about how many data requests there are? These seem to > just clutter these new tests. It seems like the verification of these counts > should be done in simple focused tests and left out of all other tests. I've dropped the checks that are redundant with simpler tests that already cover things like test condition setup. Some of these new cases have distinct setup, so I've kept some of the checks or used helpers to abstract the clutter. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1142: while (GetMediaDecoderJob(true)->is_decoding() || On 2013/12/06 18:28:30, acolwell wrote: > nit: It seems like loops like this should just be put in some sort of > WaitForDecodeDone() helper that allows you to control which streams you are > waiting for. Good point, I'll add a helper. https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1292: SeekDuringEOSDecode(true, true, false , false); Oops. This configuration mismatched test intent, and had bad style!! Fixed.
On 2013/12/09 22:58:26, wolenetz wrote: > Please take a look at patch set 6. Thanks! > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > File media/base/android/media_source_player.cc (right): > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.cc:615: PlaybackCompleted(is_audio); // > Includes ProcessPendingEvents() if needed. > On 2013/12/06 18:28:30, acolwell wrote: > > nit: Why don't you just let this fall through to the statement below? Then you > > don't need this comment or the call inside PlaybackCompleted(). This appears > to > > be the only callsite so I think it is safe. > > A little tweaking is also required to ensure that we do not proceed with > remainder of this method beyond PlaybackCompleted() + conditional > ProcessPendingEvents() after detecting output_eos here. Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.cc:764: bool > MediaSourcePlayer::AudioFinishedOrNoAudio() { > On 2013/12/06 18:28:30, acolwell wrote: > > nit: I think the OrNoAudio() just confuses things at the call sites. How about > > just always initalizing 'audio_finished_ = !HasAudio()' everywhere you > > 'audio_finished_ = false;' > > > > You could put this in a ResetFinishedFlags() helper so you don't have to > > duplicate the code & explanitory comments. > > This makes sense with the current *_finished_ flags' naming, though additional > tweaking of OnDemuxerConfigsAvailable() would be necessary: Has{Audio,Video}() > potentially change across configuration change, from initial construction to > changing the {audio, video} codec from unknown. > > An alternative approach is to rename *_finished_ to reached_*_eos_, and > *FinishedOrNoVideo() to just *Finished(), without altering anything in the > config change handling (since > 1) eos cannot be reached for an unknown codec, and > 2) everywhere we want to know if a stream is "finished", if the stream is > unconfigured, we expect it to be finished). > > I've chosen the latter (renaming) approach. Outside of constructor > initialization, these reached_*_eos_ members are only set to false in > OnDemuxerSeekDone(), so I have not added a reset eos helper. Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.cc:768: bool > MediaSourcePlayer::VideoFinishedOrNoVideo() { > On 2013/12/06 18:28:30, acolwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.cc:954: DCHECK(audio_decoder_job_); > On 2013/12/06 18:28:30, acolwell wrote: > > nit: This seems redundant since DecodeMoreAudio() will crash if this isn't > true. > > I'd rather have it crash there then leak preconditions to this callsite. > > Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.cc:959: DCHECK(video_decoder_job_); > On 2013/12/06 18:28:30, acolwell wrote: > > ditto > > Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > File media/base/android/media_source_player.h (right): > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player.h:122: // Functions that check whether > audio/video decode has reached end of output > On 2013/12/06 20:34:47, qinmin wrote: > > s/decode/stream/? > > Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:567: EXPECT_TRUE(have_audio > || have_video); > On 2013/12/06 18:28:30, acolwell wrote: > > These should be DCHECKs since these are verifying preconditions. > > Done. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:745: SeekPlayer(true, > base::TimeDelta(), true, 1); > On 2013/12/06 18:28:30, acolwell wrote: > > nit: add and overload or default values so this type of change isn't needed > here > > and below. > > Done. > Note > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... > disallows usage of default argument values for this case, so I used an overload. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:989: > TEST_F(MediaSourcePlayerTest, SecondVideoDataIsEOSAndSeekPlusStartResumesPlay) { > On 2013/12/06 20:34:47, qinmin wrote: > > the test name is hard to understand. What about > > SecondVideoAccessUnitIsEOSAndResumePlayAfterSeek > > Done, using {A,V,AV,VA} prefixes across these new/changed tests for consistency > and line length. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1123: EXPECT_EQ(5, > demuxer_->num_data_requests()); > On 2013/12/06 18:28:30, acolwell wrote: > > nit: Do we really care about how many data requests there are? These seem to > > just clutter these new tests. It seems like the verification of these counts > > should be done in simple focused tests and left out of all other tests. > > I've dropped the checks that are redundant with simpler tests that already cover > things like test condition setup. Some of these new cases have distinct setup, > so I've kept some of the checks or used helpers to abstract the clutter. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1142: while > (GetMediaDecoderJob(true)->is_decoding() || > On 2013/12/06 18:28:30, acolwell wrote: > > nit: It seems like loops like this should just be put in some sort of > > WaitForDecodeDone() helper that allows you to control which streams you are > > waiting for. > > Good point, I'll add a helper. > > https://codereview.chromium.org/79283006/diff/170001/media/base/android/media... > media/base/android/media_source_player_unittest.cc:1292: > SeekDuringEOSDecode(true, true, false , false); > Oops. This configuration mismatched test intent, and had bad style!! > Fixed. acolwell@, ping pretty please :)
It still feels like there is a lot of unnecessary expectations and duplicate code here. It feels like the MockDemuxer should be doing more, so that each test case doesn't have to keep manually handling responses to data, config, and seek requests. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:648: WaitForDecodeDone(eos_is_audio, !eos_is_audio, !eos_is_audio, eos_is_audio); This is really confusing looking. Do you really need the spot checks. If a decoderjob gets called after it signals EOS won't it just DCHECK? If so then I don't think you actually need to have this spot check logic because the test will crash if the wrong thing happens. I'd rather rely on that then have the spot checking here in the tests. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:659: SeekPlayer(true /* ignored, since not aborting */, nit: I think this just makes this unnecessarily confusing. Can't you just put the following here and revert the SeekPlayer() changes. player_.SeekTo( base::TimeDelta()); player_.OnDemuxerSeekDone(kNoTimestamp()); I think this would be much easier to understand. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:666: EXPECT_TRUE((GetMediaDecoderJob(true) != NULL) == have_audio && Shouldn't this type of check be in Start() instead of here? https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:669: demuxer_->num_data_requests()); Can this be moved into start as well? https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:690: EXPECT_TRUE((GetMediaDecoderJob(true) != NULL) == have_audio && ditto. here and elsewhere. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1130: EXPECT_TRUE(GetMediaDecoderJob(false)); ditto https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1156: EXPECT_TRUE(GetMediaDecoderJob(true) && GetMediaDecoderJob(false)); ditto. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1185: EXPECT_TRUE(GetMediaDecoderJob(true) && GetMediaDecoderJob(false)); ditto. here and elsewhere
Thanks for comments. I've cleaned up the tests a bit along the lines of your comments, and fixed a few comment and one test code error in the process. I've also filed http://crbug.com/327839 to get further test cleanup done (improve mock usage + fix flaky MDJ pointer inequality testing). acolwell@, please take a look at patch set 7. qinmin@, fyi there have been a few changes impacting other MSP unit tests since your previous l*tm; please also review PS7. Thanks! https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:504: EXPECT_EQ(expected_num_data_requests, demuxer_->num_seek_requests()); Oops. This typo (num_*seek?!*_requests) is fixed in next patch set. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:648: WaitForDecodeDone(eos_is_audio, !eos_is_audio, !eos_is_audio, eos_is_audio); On 2013/12/11 21:20:22, acolwell wrote: > This is really confusing looking. Do you really need the spot checks. If a > decoderjob gets called after it signals EOS won't it just DCHECK? If so then I > don't think you actually need to have this spot check logic because the test > will crash if the wrong thing happens. I'd rather rely on that then have the > spot checking here in the tests. Yeah, this was confusing because it was more of a black-box approach that actually found issues with an earlier patch set. With current patch set, yes, MDJ would just DCHECK (unless a new MDJ were somehow created, resetting MDJ's output_eos detection flag), so I've simplified this test helper. Done. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:659: SeekPlayer(true /* ignored, since not aborting */, On 2013/12/11 21:20:22, acolwell wrote: > nit: I think this just makes this unnecessarily confusing. Can't you just put > the following here and revert the SeekPlayer() changes. > > player_.SeekTo( base::TimeDelta()); > player_.OnDemuxerSeekDone(kNoTimestamp()); > > I think this would be much easier to understand. Yes, done :) https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:666: EXPECT_TRUE((GetMediaDecoderJob(true) != NULL) == have_audio && On 2013/12/11 21:20:22, acolwell wrote: > Shouldn't this type of check be in Start() instead of here? Start() calls MSP::Start(), which does not guarantee that jobs are created. For instance, video decoder job creation could be pending a valid surface, or audio/video decoder job(s) creation could be pending OnDemuxerSeekDone(). Perhaps a parameterized version of the Start() helper could simplify this; I'll take a stab at this to see what it might look like. I've also filed https://crbug.com/327839 to try to get much of this clutter moved into mocks, and also to get some flaky pointer-based MDJ inequality testing fixed. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:669: demuxer_->num_data_requests()); On 2013/12/11 21:20:22, acolwell wrote: > Can this be moved into start as well? Along the lines of similar comment, I'll take a stab at this. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:690: EXPECT_TRUE((GetMediaDecoderJob(true) != NULL) == have_audio && On 2013/12/11 21:20:22, acolwell wrote: > ditto. here and elsewhere. Done. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:820: // Test video decoder job will be created when surface is valid. This comment is incorrect. s/will be/will not be/, and s/valid/invalid/ will be included in next patch set. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1031: // Sending video data to player, audio decoder should not start. This comment is incorrect. s/audio/video/ is included in next patch set. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1130: EXPECT_TRUE(GetMediaDecoderJob(false)); On 2013/12/11 21:20:22, acolwell wrote: > ditto Done. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1156: EXPECT_TRUE(GetMediaDecoderJob(true) && GetMediaDecoderJob(false)); On 2013/12/11 21:20:22, acolwell wrote: > ditto. Done. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1185: EXPECT_TRUE(GetMediaDecoderJob(true) && GetMediaDecoderJob(false)); On 2013/12/11 21:20:22, acolwell wrote: > ditto. here and elsewhere Done. https://codereview.chromium.org/79283006/diff/220001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1998: // request OnDemuxerSeekDone() does not occur until after the next Start(), s/request OnDemux/request and OnDemux/ comment fix is included in next patch set.
lgtm % nit https://codereview.chromium.org/79283006/diff/260001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/260001/media/base/android/media... media/base/android/media_source_player_unittest.cc:628: DCHECK(have_audio || !eos_audio); nit: this feels redundant. If have_audio is false, we can just ignore the eos_audio value
acolwell, ptal @ ps8 https://codereview.chromium.org/79283006/diff/260001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/79283006/diff/260001/media/base/android/media... media/base/android/media_source_player_unittest.cc:628: DCHECK(have_audio || !eos_audio); On 2013/12/12 19:40:22, qinmin wrote: > nit: this feels redundant. If have_audio is false, we can just ignore the > eos_audio value Done.
lgtm
On 2013/12/12 21:36:06, acolwell wrote: > lgtm Thanks acolwell & qinmin. I'll send this to CQ shortly.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/79283006/280001
Message was sent while issue was closed.
Change committed as 240500 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
