Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
53413004:
Clear any pending surface change prior to checking media crypto in MSP::ConfigureVideoDecoderJob() (Closed)
|
Created:
7 years, 1 month ago by wolenetz Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClear any pending surface change prior to checking media crypto in MSP::ConfigureVideoDecoderJob()
Ensures that any valid invocation of ConfigureVideoDecoderJob with a
pending surface change event causes the job to be reset and the
surface change event cleared immediately, or after a possible browser seek.
Also changes call from ConfigureVideoDecoderJob() to |manager_|->OnMediaMetaDataChanged() to only occur if new job was created.
BUG=313470
R=xhwang@chromium.org,qinmin@chromium.org,acolwell@chromium.org
TEST=all media_unittests pass on Android with MediaCodecBridge available and the new test fails prior to patching MSP
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232897
Patch Set 1 #
Total comments: 11
Patch Set 2 : Move job reset and clearing any pending surface change event after the browser seek logic #
Total comments: 1
Patch Set 3 : Addresses comments and adds a narrow unit test #
Total comments: 2
Patch Set 4 : add reference from new unit test to http://crbug.com/313860 #
Total comments: 4
Patch Set 5 : Address nits from PS4 #Patch Set 6 : rebase #
Messages
Total messages: 20 (0 generated)
PTAL: xhwang@: Does this fix http://crbug.com/313470? qinmin@: OWNERS review please. acolwell@: review if you wish, too :)
One question I forgot to ask earlier: https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); Note, if is_video_encrypted but there is no valid media_crypto yet, the job is reset earlier with this change. Is this problematic?
https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); Should be ok. If there is no media_crypto, the job cannot be created. On 2013/10/31 03:03:09, wolenetz wrote: > Note, if is_video_encrypted but there is no valid media_crypto yet, the job is > reset earlier with this change. Is this problematic? https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:747: if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING)) i think all this should happen after the next if statement . Seek should take priority above surface change. After seek is completed, we will call processPendingEvents and enter here.
Does this cause an externally visible change in behavior? (ie Can code outside MSP detect this change?) https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); ISTM that we should create a helper function for resetting the video_decoder_job_ so that we always clear the surface change event whenever the job is destroyed. I think this would remove duplicate logic along these lines sprinkled around this file. https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:785: manager()->OnMediaMetadataChanged( Why do we blindly notify the manager even when the video_decoder_job_ isn't created?
Thanks, qinmin@. All, PTAL @ PS2. https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); On 2013/10/31 16:35:52, qinmin wrote: > Should be ok. If there is no media_crypto, the job cannot be created. > > On 2013/10/31 03:03:09, wolenetz wrote: > > Note, if is_video_encrypted but there is no valid media_crypto yet, the job is > > reset earlier with this change. Is this problematic? > Could there be a regression here, too: Could we previously have media_crypto and a video_decoder_job_ (or is_video_encrypted_ was false and we had a job, but later is_video_encrypted_ became true), such that previous behavior would be to not reset the job until we get non-null media_crypto? Or is it always ok to reset the job this early? https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:747: if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING)) On 2013/10/31 16:35:52, qinmin wrote: > i think all this should happen after the next if statement . Seek should take > priority above surface change. After seek is completed, we will call > processPendingEvents and enter here. Done. In combination with eventual landing of https://codereview.chromium.org/51613002/, Release() will preserve the pending seek (including browser seek) status, too, and still clear any pending surface change event.
https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); we always need to reset the vide_decoder_job if media_crypto changes. It doesn't matter where we reset it. On 2013/10/31 18:12:07, wolenetz wrote: > On 2013/10/31 16:35:52, qinmin wrote: > > Should be ok. If there is no media_crypto, the job cannot be created. > > > > On 2013/10/31 03:03:09, wolenetz wrote: > > > Note, if is_video_encrypted but there is no valid media_crypto yet, the job > is > > > reset earlier with this change. Is this problematic? > > > > Could there be a regression here, too: Could we previously have media_crypto and > a video_decoder_job_ (or is_video_encrypted_ was false and we had a job, but > later is_video_encrypted_ became true), such that previous behavior would be to > not reset the job until we get non-null media_crypto? Or is it always ok to > reset the job this early? https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:785: manager()->OnMediaMetadataChanged( If the decoder job is created, this is to let the fullscreen video view know that Media metadata is available. When swiching to fullscreen, we show a circling animation to indicate that the player is not ready. When metadata is available, we call this so that contentVideoView know when to start showing the video. However, we don't need to do this if video_decoder_job_ is not created. I think originally VideoDecoderJob::Create() always return a non-null pointer. But since it can return a null pointer now, so we need to move this statement into the above if statement to avoid removing the animation On 2013/10/31 18:11:28, acolwell wrote: > Why do we blindly notify the manager even when the video_decoder_job_ isn't > created?
I tested this CL and it fixes crbug.com/313470
Next patch set will include updated unit test per chat with qinmin@. PTAL @ MSP.* in PS3. https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:745: video_decoder_job_.reset(); On 2013/10/31 18:11:28, acolwell wrote: > ISTM that we should create a helper function for resetting the > video_decoder_job_ so that we always clear the surface change event whenever the > job is destroyed. I think this would remove duplicate logic along these lines > sprinkled around this file. Done, added helper MSP::ResetVideoDecoderJob(). https://codereview.chromium.org/53413004/diff/1/media/base/android/media_sour... media/base/android/media_source_player.cc:785: manager()->OnMediaMetadataChanged( On 2013/10/31 18:32:37, qinmin wrote: > If the decoder job is created, this is to let the fullscreen video view know > that Media metadata is available. > When swiching to fullscreen, we show a circling animation to indicate that the > player is not ready. When metadata is available, we call this so that > contentVideoView know when to start showing the video. > > However, we don't need to do this if video_decoder_job_ is not created. I think > originally VideoDecoderJob::Create() always return a non-null pointer. But since > it can return a null pointer now, so we need to move this statement into the > above if statement to avoid removing the animation > > > On 2013/10/31 18:11:28, acolwell wrote: > > Why do we blindly notify the manager even when the video_decoder_job_ isn't > > created? > Done. https://codereview.chromium.org/53413004/diff/90001/media/base/android/media_... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/90001/media/base/android/media_... media/base/android/media_source_player.cc:326: manager()->OnMediaMetadataChanged( Note, this already occurs prior to |video_decoder_job_| creation attempts in ConfigureVideoDecoderJob() (which will be unsuccessful until configs arrive), so the ContentVideoView could stop pending player readiness (to stop animation and setup controls) even before successful |video_decoder_job_| creation. This is not a regression though. https://codereview.chromium.org/53413004/diff/190001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/53413004/diff/190001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1388: // TODO(xhwang): Once we add tests to cover DrmBridge, update this test to From chat with qinmin@, MSP should be able to create the job with just media crypto injected; no drm bridge should be necessary. I'll add to this test in the next patch set.
PTAL @ PS4; I believe I've addressed all comments so far. Also note that the new unit test fails as expected on an unpatched MSP: [FAIL] MediaSourcePlayerTest.SurfaceChangeClearedEvenIfMediaCryptoAbsent: [VERBOSE1:media_source_player.cc(304)] OnDemuxerConfigsAvailable [VERBOSE1:media_source_player.cc(881)] SetPendingEvent(SURFACE_CHANGE) [VERBOSE1:media_source_player.cc(461)] ProcessPendingEvents : 0x2 [VERBOSE1:media_source_player.cc(494)] ProcessPendingEvents : Handling SURFACE_CHANGE_EVENT. ../../media/base/android/media_source_player_unittest.cc:1404: Failure Value of: IsPendingSurfaceChange() Actual: true Expected: false [VERBOSE1:media_source_player.cc(232)] Release https://codereview.chromium.org/53413004/diff/190001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/53413004/diff/190001/media/base/android/media... media/base/android/media_source_player_unittest.cc:1388: // TODO(xhwang): Once we add tests to cover DrmBridge, update this test to On 2013/10/31 20:34:25, wolenetz wrote: > From chat with qinmin@, MSP should be able to create the job with just media > crypto injected; no drm bridge should be necessary. I'll add to this test in the > next patch set. Actually, MSP::GetMediaCrypto() requires previous MSP::SetDrmBridge(). And the suggested expansion of this test requires MSP::GetMediaCrypto() to return non-null media crypto usable on the test device to create the job. Hence the existing TODO. I've modified the TODO to also reference a bug that suggests routes to inject a media crypto for test usage, outside the scope of this CL.
lgtm MediaSourcePlayer::ConfigureVideoDecoderJob() is getting really complicated. If only we could simplify it somehow... https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... media/base/android/media_source_player.cc:747: DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding()); Do we ever call ConfigureVideoDecoderJob() when video_decoder_job_->is_decoding()? If not, put this check at the begining of this function? It's a bit confusing to DCHECK this after so many "if" blocks. Or am I missing anything?
lgtm % nit https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... media/base/android/media_source_player.cc:782: if (video_decoder_job_) { nit: reverse condition and return early.
lgtm
Thanks for all the comments. After https://codereview.chromium.org/51613002/ lands, I'll rebase this and send to CQ. https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... media/base/android/media_source_player.cc:747: DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding()); On 2013/10/31 21:37:18, xhwang wrote: > Do we ever call ConfigureVideoDecoderJob() when > video_decoder_job_->is_decoding()? If not, put this check at the begining of > this function? It's a bit confusing to DCHECK this after so many "if" blocks. Or > am I missing anything? Good point. Thanks for chat that helped clarify EME scenarios, and (current) lack of their MSP::[EME config method]()->StartInternal()->CVDJ() possibly racing a current video decoder job decode of previously received data. Details: 1. OnKeyAdded()->CVDJ() cannot race an existing decode, even though it can arrive any time. |is_waiting_for_key_| is reset in StartInternal() and protects against calling StartInternal() from OnKeyAdded(). 2. OnMediaCryptoReady() must occur prior to initial encrypted video job's successful creation. SBS::UpdateVideoConfig() prevents subsequent change of video's is_encrypted. "For now, we only call OnMediaCryptoReady() once." 3. SetDrmBridge(): "For now, we only call SetDrmBridge() once." Given the EME spec might change or other refactoring could introduce other race vectors, keeping the DCHECK after our existing protection against reconfiguring an existing job that we're not expecting to reconfigure (|reconfig_video_decoder_|) seems the safest approach for now. Hence, I'll move the DCHECK no earlier than after the second if() clause in this CL. https://codereview.chromium.org/53413004/diff/240001/media/base/android/media... media/base/android/media_source_player.cc:782: if (video_decoder_job_) { On 2013/10/31 21:45:08, acolwell wrote: > nit: reverse condition and return early. Done. This is a pattern I'll start watching for more proactively :) Thanks.
Thanks for all the comments. After https://codereview.chromium.org/51613002/ lands, I'll rebase this and send to CQ.
On 2013/11/02 00:01:03, wolenetz wrote: > Thanks for all the comments. > After https://codereview.chromium.org/51613002/ lands, I'll rebase this and send > to CQ. PS6 rebases PS5. I've confirmed TEST=... details again locally and am sending PS6 to CQ shortly. Thanks everyone.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/53413004/400001
Retried try job too often on win_rel for step(s) content_unittests 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/53413004/400001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/53413004/400001
Message was sent while issue was closed.
Change committed as 232897 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
