Closed Bug 1194606 Opened 9 years ago Closed 9 years ago

Make MediaDecoderStateMachine capable of requesting different kind (decoded/raw) of media data.

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
FxOS-S9 (16Oct)
Project Flags:
feature-b2g 2.5+
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: kikuo, Assigned: JamesCheng)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [ft:conndevices][partner-blocker])

Attachments

(2 files, 24 obsolete files)

Part1-Make-MediaDecoderStateMachine-capable-of.patch
10.97 KB, patch
JamesCheng
: review+
JamesCheng
: feedback+
Details | Diff | Splinter Review
Part2-Add-gtest-for-demuxed-only-scenario-in-M.patch
9.90 KB, patch
JamesCheng
: review+
JamesCheng
: feedback+
Details | Diff | Splinter Review
Based on Bug 1188812, we're able to know if these media data required to be (decrypted)/decoded/rendered externally. So MDSM shall be able to request corresponding media data in related to different type of media sinks and media data consumer.
Blocks: 1146795
May refer to Attachment 8625856 [details] [diff] as a prototype in a hacky way.
Let me work on this!
Assignee: nobody → jacheng
Priority: -- → P1
Target Milestone: --- → FxOS-S7 (18Sep)
Blocks: TV_FxOS2.5
Status: NEW → ASSIGNED
feature-b2g: --- → 2.5+
Hi JW and Kilik, Could you please give me some suggestions for this patch? The idea is to keep the original implementation and use TrackInfo::mIsRenderedExternally to determine if we need to demux-only or do the demux-and-decode for the a/v samples. And I modified the API naming |Decode| to general name |Process|. If it is the right design direction, should anything need to be considered that I ignore in this patch? Thanks.
Attachment #8654792 - Flags: feedback?(kikuo)
Attachment #8654792 - Flags: feedback?(jwwang)
Comment on attachment 8654792 [details] [diff] [review] [WIP] Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8654792 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1638,5 @@ > mAudioDataRequest.Begin(InvokeAsync(DecodeTaskQueue(), mReader.get(), > __func__, &MediaDecoderReader::RequestAudioData) > ->Then(OwnerThread(), __func__, this, > + &MediaDecoderStateMachine::OnAudioProcessed, > + &MediaDecoderStateMachine::OnAudioNotProcessed)); Why not splitting the logic of requesting different type of data here by creating another resovle/reject function ? |OnAudioDecoded()| takes AudioData as input but |OnAudioProcessed()| will handle AudioData or MediaRawData. For clear development and better maintenance, I don't think they should share same logic. @@ +1723,5 @@ > &MediaDecoderReader::RequestVideoData, > skipToNextKeyFrame, currentTime, forceDecodeAhead) > ->Then(OwnerThread(), __func__, this, > + &MediaDecoderStateMachine::OnVideoProcessed, > + &MediaDecoderStateMachine::OnVideoNotProcessed)); ditto ::: dom/media/MediaFormatReader.cpp @@ +1054,5 @@ > + > + // Demuxed only since it is decoded by external rendering module. > + // Callback directly with demuxed raw data. > + if ((*info)->mIsRenderedExternally) { > + decoder.mCallback->Output(sample); We shouldn't return demuxed sample in |DecodedDemuxedSamples()|, that will make this function ambiguous.
Attachment #8654792 - Flags: feedback?(kikuo)
Comment on attachment 8654792 [details] [diff] [review] [WIP] Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8654792 [details] [diff] [review]: ----------------------------------------------------------------- Per discussion, I think MediaDecoderReader should provide 2 more functions like Request{Audio,Video}RawData to return demuxed (which could be encrypted) samples to MDSM. And MDSM has its own policy to decide whether to request decoded or demuxed data from the reader. You should discuss the design changes in MediaDecoderReader with jya when he is back from PTO.
Attachment #8654792 - Flags: feedback?(jwwang)
Hello JW and Kilik, After discussion, I added Request{Audio,Video}RawData APIs in MDSM and MediaDecoderReader. Since the current design is for |demux then decode|, I encounter some incompatible conditions. Please help me for some feedbacks of this patch. If there is anything that did not need to modify in this patch, please let me know. Thank you very much.
Attachment #8658054 - Flags: feedback?(kikuo)
Attachment #8658054 - Flags: feedback?(jwwang)
Comment on attachment 8654792 [details] [diff] [review] [WIP] Make-MediaDecoderStateMachine-capable-of.patch ># HG changeset patch ># User James Cheng <jacheng@mozilla.com> > >Bug 1194606 - Make MediaDecoderStateMachine capable of requesting different kind (decoded/raw) of media data. > > >diff --git a/dom/media/MediaDecoderStateMachine.cpp b/dom/media/MediaDecoderStateMachine.cpp >index eb21e12..0e4f5f5 100644 >--- a/dom/media/MediaDecoderStateMachine.cpp >+++ b/dom/media/MediaDecoderStateMachine.cpp >@@ -529,27 +529,27 @@ MediaDecoderStateMachine::IsVideoSeekComplete() > return > !HasVideo() || > (mCurrentSeek.Exists() && > !mDropVideoUntilNextDiscontinuity && > (VideoQueue().IsFinished() || VideoQueue().GetSize() > 0)); > } > > void >-MediaDecoderStateMachine::OnAudioDecoded(AudioData* aAudioSample) >+MediaDecoderStateMachine::OnAudioProcessed(AudioData* aAudioSample) > { > MOZ_ASSERT(OnTaskQueue()); > ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor()); > nsRefPtr<AudioData> audio(aAudioSample); > MOZ_ASSERT(audio); > mAudioDataRequest.Complete(); > aAudioSample->AdjustForStartTime(StartTime()); > mDecodedAudioEndTime = audio->GetEndTime(); > >- SAMPLE_LOG("OnAudioDecoded [%lld,%lld] disc=%d", >+ SAMPLE_LOG("OnAudioProcessed [%lld,%lld] disc=%d", > (audio ? audio->mTime : -1), > (audio ? audio->GetEndTime() : -1), > (audio ? audio->mDiscontinuity : 0)); > > switch (mState) { > case DECODER_STATE_BUFFERING: { > // If we're buffering, this may be the sample we need to stop buffering. > // Save it and schedule the state machine. >@@ -678,22 +678,22 @@ MediaDecoderStateMachine::OnVideoPopped(const nsRefPtr<MediaData>& aSample) > MOZ_ASSERT(OnTaskQueue()); > ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor()); > mDecoder->UpdatePlaybackOffset(aSample->mOffset); > UpdateNextFrameStatus(); > DispatchVideoDecodeTaskIfNeeded(); > } > > void >-MediaDecoderStateMachine::OnNotDecoded(MediaData::Type aType, >- MediaDecoderReader::NotDecodedReason aReason) >+MediaDecoderStateMachine::OnNotProcessed(MediaData::Type aType, >+ MediaDecoderReader::NotDecodedReason aReason) > { > MOZ_ASSERT(OnTaskQueue()); > ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor()); >- SAMPLE_LOG("OnNotDecoded (aType=%u, aReason=%u)", aType, aReason); >+ SAMPLE_LOG("OnNotProcessed (aType=%u, aReason=%u)", aType, aReason); > bool isAudio = aType == MediaData::AUDIO_DATA; > MOZ_ASSERT_IF(!isAudio, aType == MediaData::VIDEO_DATA); > > if (isAudio) { > mAudioDataRequest.Complete(); > } else { > mVideoDataRequest.Complete(); > } >@@ -802,27 +802,27 @@ MediaDecoderStateMachine::MaybeFinishDecodeFirstFrame() > // We can now complete the pending seek. > mPendingSeek.Steal(mQueuedSeek); > SetState(DECODER_STATE_SEEKING); > ScheduleStateMachine(); > return true; > } > > void >-MediaDecoderStateMachine::OnVideoDecoded(VideoData* aVideoSample) >+MediaDecoderStateMachine::OnVideoProcessed(VideoData* aVideoSample) > { > MOZ_ASSERT(OnTaskQueue()); > ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor()); > nsRefPtr<VideoData> video(aVideoSample); > MOZ_ASSERT(video); > mVideoDataRequest.Complete(); > aVideoSample->AdjustForStartTime(StartTime()); > mDecodedVideoEndTime = video ? video->GetEndTime() : mDecodedVideoEndTime; > >- SAMPLE_LOG("OnVideoDecoded [%lld,%lld] disc=%d", >+ SAMPLE_LOG("OnVideoProcessed [%lld,%lld] disc=%d", > (video ? video->mTime : -1), > (video ? video->GetEndTime() : -1), > (video ? video->mDiscontinuity : 0)); > > switch (mState) { > case DECODER_STATE_BUFFERING: { > // If we're buffering, this may be the sample we need to stop buffering. > // Save it and schedule the state machine. >@@ -1633,29 +1633,29 @@ MediaDecoderStateMachine::RequestAudioData() > > SAMPLE_LOG("Queueing audio task - queued=%i, decoder-queued=%o", > AudioQueue().GetSize(), mReader->SizeOfAudioQueueInFrames()); > > if (mSentFirstFrameLoadedEvent) { > mAudioDataRequest.Begin(InvokeAsync(DecodeTaskQueue(), mReader.get(), > __func__, &MediaDecoderReader::RequestAudioData) > ->Then(OwnerThread(), __func__, this, >- &MediaDecoderStateMachine::OnAudioDecoded, >- &MediaDecoderStateMachine::OnAudioNotDecoded)); >+ &MediaDecoderStateMachine::OnAudioProcessed, >+ &MediaDecoderStateMachine::OnAudioNotProcessed)); > } else { > mAudioDataRequest.Begin( > InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, > &MediaDecoderReader::RequestAudioData) > ->Then(OwnerThread(), __func__, mStartTimeRendezvous.get(), > &StartTimeRendezvous::ProcessFirstSample<AudioDataPromise>, > &StartTimeRendezvous::FirstSampleRejected<AudioData>) > ->CompletionPromise() > ->Then(OwnerThread(), __func__, this, >- &MediaDecoderStateMachine::OnAudioDecoded, >- &MediaDecoderStateMachine::OnAudioNotDecoded) >+ &MediaDecoderStateMachine::OnAudioProcessed, >+ &MediaDecoderStateMachine::OnAudioNotProcessed) > ); > } > } > > nsresult > MediaDecoderStateMachine::DispatchVideoDecodeTaskIfNeeded() > { > MOZ_ASSERT(OnTaskQueue()); >@@ -1718,30 +1718,30 @@ MediaDecoderStateMachine::RequestVideoData() > currentTime); > > if (mSentFirstFrameLoadedEvent) { > mVideoDataRequest.Begin( > InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, > &MediaDecoderReader::RequestVideoData, > skipToNextKeyFrame, currentTime, forceDecodeAhead) > ->Then(OwnerThread(), __func__, this, >- &MediaDecoderStateMachine::OnVideoDecoded, >- &MediaDecoderStateMachine::OnVideoNotDecoded)); >+ &MediaDecoderStateMachine::OnVideoProcessed, >+ &MediaDecoderStateMachine::OnVideoNotProcessed)); > } else { > mVideoDataRequest.Begin( > InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, > &MediaDecoderReader::RequestVideoData, > skipToNextKeyFrame, currentTime, forceDecodeAhead) > ->Then(OwnerThread(), __func__, mStartTimeRendezvous.get(), > &StartTimeRendezvous::ProcessFirstSample<VideoDataPromise>, > &StartTimeRendezvous::FirstSampleRejected<VideoData>) > ->CompletionPromise() > ->Then(OwnerThread(), __func__, this, >- &MediaDecoderStateMachine::OnVideoDecoded, >- &MediaDecoderStateMachine::OnVideoNotDecoded)); >+ &MediaDecoderStateMachine::OnVideoProcessed, >+ &MediaDecoderStateMachine::OnVideoNotProcessed)); > } > } > > void > MediaDecoderStateMachine::StartAudioSink() > { > MOZ_ASSERT(OnTaskQueue()); > AssertCurrentThreadInMonitor(); >diff --git a/dom/media/MediaDecoderStateMachine.h b/dom/media/MediaDecoderStateMachine.h >index 09c73c9..f7aad8a 100644 >--- a/dom/media/MediaDecoderStateMachine.h >+++ b/dom/media/MediaDecoderStateMachine.h >@@ -360,28 +360,28 @@ public: > > // Make sure that this arrives before playback starts, otherwise this won't > // have the intended effect. > MOZ_DIAGNOSTIC_ASSERT(self->mPlayState == MediaDecoder::PLAY_STATE_LOADING); > }); > OwnerThread()->Dispatch(r.forget()); > } > >- void OnAudioDecoded(AudioData* aSample); >- void OnVideoDecoded(VideoData* aSample); >- void OnNotDecoded(MediaData::Type aType, MediaDecoderReader::NotDecodedReason aReason); >- void OnAudioNotDecoded(MediaDecoderReader::NotDecodedReason aReason) >+ void OnAudioProcessed(AudioData* aSample); >+ void OnVideoProcessed(VideoData* aSample); >+ void OnNotProcessed(MediaData::Type aType, MediaDecoderReader::NotDecodedReason aReason); >+ void OnAudioNotProcessed(MediaDecoderReader::NotDecodedReason aReason) > { > MOZ_ASSERT(OnTaskQueue()); >- OnNotDecoded(MediaData::AUDIO_DATA, aReason); >+ OnNotProcessed(MediaData::AUDIO_DATA, aReason); > } >- void OnVideoNotDecoded(MediaDecoderReader::NotDecodedReason aReason) >+ void OnVideoNotProcessed(MediaDecoderReader::NotDecodedReason aReason) > { > MOZ_ASSERT(OnTaskQueue()); >- OnNotDecoded(MediaData::VIDEO_DATA, aReason); >+ OnNotProcessed(MediaData::VIDEO_DATA, aReason); > } > > // Resets all state related to decoding and playback, emptying all buffers > // and aborting all pending operations on the decode task queue. > void Reset(); > > protected: > virtual ~MediaDecoderStateMachine(); >diff --git a/dom/media/MediaFormatReader.cpp b/dom/media/MediaFormatReader.cpp >index 380ae39..d60393e 100644 >--- a/dom/media/MediaFormatReader.cpp >+++ b/dom/media/MediaFormatReader.cpp >@@ -943,18 +943,18 @@ MediaFormatReader::RequestDemuxSamples(TrackType aTrack) > if (aTrack == TrackInfo::kVideoTrack) { > DoDemuxVideo(); > } else { > DoDemuxAudio(); > } > } > > void >-MediaFormatReader::DecodeDemuxedSamples(TrackType aTrack, >- AbstractMediaDecoder::AutoNotifyDecoded& aA) >+MediaFormatReader::ProcessDemuxedSamples(TrackType aTrack, >+ AbstractMediaDecoder::AutoNotifyDecoded& aA) > { > MOZ_ASSERT(OnTaskQueue()); > auto& decoder = GetDecoderData(aTrack); > > if (decoder.mQueuedSamples.IsEmpty()) { > return; > } > >@@ -966,17 +966,17 @@ MediaFormatReader::DecodeDemuxedSamples(TrackType aTrack, > > if (!EnsureDecodersInitialized()) { > ScheduleUpdate(aTrack); > return; > } > > LOGV("Giving %s input to decoder", TrackTypeToStr(aTrack)); > >- // Decode all our demuxed frames. >+ // Decode all our demuxed frames or returning the demuxed raw data without decoding. > bool samplesPending = false; > while (decoder.mQueuedSamples.Length()) { > nsRefPtr<MediaRawData> sample = decoder.mQueuedSamples[0]; > nsRefPtr<SharedTrackInfo> info = sample->mTrackInfo; > > if (info && decoder.mLastStreamSourceID != info->GetID()) { > if (samplesPending) { > // Let existing samples complete their decoding. We'll resume later. >@@ -1046,21 +1046,31 @@ MediaFormatReader::DecodeDemuxedSamples(TrackType aTrack, > LOGV("Input:%lld (dts:%lld kf:%d)", > sample->mTime, sample->mTimecode, sample->mKeyframe); > decoder.mOutputRequested = true; > decoder.mNumSamplesInput++; > decoder.mSizeOfQueue++; > if (aTrack == TrackInfo::kVideoTrack) { > aA.mParsed++; > } >- if (NS_FAILED(decoder.mDecoder->Input(sample))) { >- LOG("Unable to pass frame to decoder"); >- NotifyError(aTrack); >- return; >+ >+ // Demuxed only since it is decoded by external rendering module. >+ // Callback directly with demuxed raw data. >+ if ((*info)->mIsRenderedExternally) { >+ decoder.mCallback->Output(sample); > } >+ // Otherwise, do the decode task. >+ else { >+ if (NS_FAILED(decoder.mDecoder->Input(sample))) { >+ LOG("Unable to pass frame to decoder"); >+ NotifyError(aTrack); >+ return; >+ } >+ } >+ > decoder.mQueuedSamples.RemoveElementAt(0); > samplesPending = true; > } > > // We have serviced the decoder's request for more data. > decoder.mInputExhausted = false; > } > >@@ -1186,17 +1196,17 @@ MediaFormatReader::Update(TrackType aTrack) > LOGV("Update(%s) ni=%d no=%d ie=%d, in:%llu out:%llu qs=%u sid:%u", > TrackTypeToStr(aTrack), needInput, needOutput, decoder.mInputExhausted, > decoder.mNumSamplesInput, decoder.mNumSamplesOutput, > uint32_t(size_t(decoder.mSizeOfQueue)), decoder.mLastStreamSourceID); > > // Demux samples if we don't have some. > RequestDemuxSamples(aTrack); > // Decode all pending demuxed samples. >- DecodeDemuxedSamples(aTrack, a); >+ ProcessDemuxedSamples(aTrack, a); > } > > void > MediaFormatReader::ReturnOutput(MediaData* aData, TrackType aTrack) > { > auto& decoder = GetDecoderData(aTrack); > MOZ_ASSERT(decoder.HasPromise()); > if (decoder.mDiscontinuity) { >diff --git a/dom/media/MediaFormatReader.h b/dom/media/MediaFormatReader.h >index 7f773c9..5324f24 100644 >--- a/dom/media/MediaFormatReader.h >+++ b/dom/media/MediaFormatReader.h >@@ -114,19 +114,19 @@ private: > // Lock for corresponding track must be held. > void ScheduleUpdate(TrackType aTrack); > void Update(TrackType aTrack); > // Handle actions should more data be received. > // Returns true if no more action is required. > bool UpdateReceivedNewData(TrackType aTrack); > // Called when new samples need to be demuxed. > void RequestDemuxSamples(TrackType aTrack); >- // Decode any pending already demuxed samples. >- void DecodeDemuxedSamples(TrackType aTrack, >- AbstractMediaDecoder::AutoNotifyDecoded& aA); >+ // Decode or just bypass any pending already demuxed samples. >+ void ProcessDemuxedSamples(TrackType aTrack, >+ AbstractMediaDecoder::AutoNotifyDecoded& aA); > // Drain the current decoder. > void DrainDecoder(TrackType aTrack); > void NotifyNewOutput(TrackType aTrack, MediaData* aSample); > void NotifyInputExhausted(TrackType aTrack); > void NotifyDrainComplete(TrackType aTrack); > void NotifyError(TrackType aTrack); > void NotifyWaitingForData(TrackType aTrack); > void NotifyEndOfStream(TrackType aTrack); >
Attachment #8654792 - Attachment is obsolete: true
Comment on attachment 8658054 [details] [diff] [review] [WIP] Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8658054 [details] [diff] [review]: ----------------------------------------------------------------- This is a contradiction of bug 1186367 where we want MDSM to be ignorant of the data returned by the reader. If your code has to tell MDSM to tell raw data from decoded data from time to time, you are probably going the wrong way.
Attachment #8658054 - Flags: feedback?(jwwang) → feedback-
(In reply to James Cheng[:JamesCheng] from comment #6) > Created attachment 8658054 [details] [diff] [review] > [WIP] Make-MediaDecoderStateMachine-capable-of.patch > > Hello JW and Kilik, > > After discussion, I added Request{Audio,Video}RawData APIs in MDSM and > MediaDecoderReader. Since the current design is for |demux then decode|, I > encounter some incompatible conditions. > > Please help me for some feedbacks of this patch. > > If there is anything that did not need to modify in this patch, please let > me know. > > Thank you very much. Another way is to have MediaDecoderReader::Request{Audio,Video}Data return MediaData which could be decoded or demuxed depending on how the reader is configured beforehand. You should get suggestions from jya who knows the reader better.
Comment on attachment 8658054 [details] [diff] [review] [WIP] Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8658054 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +623,4 @@ > } else { > // We're doing an accurate seek. We must discard > // MediaData up to the one containing exact seek target. > + if (NS_FAILED(DropAudioUpToSeekTarget(aAudioSamle))) { aAudioSam"p"le and several ditto above. @@ +640,5 @@ > } > > void > +MediaDecoderStateMachine::Push(MediaData* aSample, > + MediaQueue<MediaData>& aQueue) To identify which type of MediaData can be done directly in MDSM::Push(MediaData* aSample) by Media_Data::Type and mTrackInfo of MediaRawData. There's no need to add another "aQueue" parameter for this function. This also made you placing AudioQueue() / VideoQueue() in many places. Also, you still need to check the type in your current design, isn't it ? :) @@ +669,5 @@ > + // do cast inevitablely? > + if (aSample->mType == MediaData::VIDEO_DATA) { > + aSample->As<VideoData>()->mFrameID = ++mCurrentFrameID; > + } > + aQueue.PushFront(aSample); ditto @@ +725,4 @@ > bool isAudio = aType == MediaData::AUDIO_DATA; > MOZ_ASSERT_IF(!isAudio, aType == MediaData::VIDEO_DATA); > > + if (aIsRawData) { I think what made MDSM requesting raw data can also be used here to identify the case encountered. So it seems you don't need aIsRawData here. ::: dom/media/MediaDecoderStateMachine.h @@ +499,5 @@ > // If aTimeStamp is non-null, set *aTimeStamp to the TimeStamp corresponding > // to the returned stream time. > int64_t GetClock(TimeStamp* aTimeStamp = nullptr) const; > > + nsresult DropAudioUpToSeekTarget(MediaData* aSample); I'm not quite sure if it's good idea to modify the type here. Please see the last issues in Bug 1146795 Comment 20. @@ +631,5 @@ > + RejectMethod&& aRejecter, > + Executor&& aExecutor, > + Args&&... aArgs) > + { > + if (mSentFirstFrameLoadedEvent) { IMHO, it doesn't seem to be a good choice to encapsulate codes into template pattern when some member variables involve in. It would make change of logic more complicated in the future if these variables get involved. ::: dom/media/MediaFormatReader.cpp @@ +1053,5 @@ > + return true; > +} > + > +void > +MediaFormatReader::HandleDemuxedSamples(TrackType aTrack, I think keeping this function named "DecodeDemuxedSamples" would be more proper and easier to understand. For now, only 2 cases for MFR to return MediaData back to requester (1 is demuxed, 1 is demuxed+decoded) @@ +1160,5 @@ > if (aTrack == TrackInfo::kVideoTrack) { > aA.mParsed++; > } > + // Run the designated behavior for this sample. > + if ((this->*aHandleSampleBehavior)(aTrack, sample)) { If there's no other behavior in this function, do we need this ?
Attachment #8658054 - Flags: feedback?(kikuo)
Depends on: 1203047
Thanks for the feedbacks. I create a precondition Bug 1203047, I should finish this bug first so I will be able to continue to work on this bug.
Hi JW, Per discussion, I add the API to query if reader supports demuxed-only ability and a setter to set the demuxed-only flag to reader. MDSM's interface is not changed. Only config the reader when the metadata is retrieved. The MediaFormatReader can determine how to handle the demuxed data(decode or resolve directly to statemachine). Please help to give me some feedback if anything I ignored to handle. Thanks.
Attachment #8658054 - Attachment is obsolete: true
Attachment #8659812 - Flags: feedback?(jwwang)
Comment on attachment 8659812 [details] [diff] [review] v1-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8659812 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderReader.h @@ +167,5 @@ > + // if data request is decoded or demuxed only. > + void SetDemuxedOnly(bool aDemuxedOnly) { > + mDemuxedOnly = true; > + } > + bool IsDemuxedOnly() { It doesn't make sense to return true if a reader doesn't support demux-only mode. SetDemuxedOnly() and IsDemuxedOnly() should be empty here and leave the implementation to sub-classes. @@ +437,5 @@ > bool mHitAudioDecodeError; > bool mShutdown; > > + // Set the demuxed-only flag. > + bool mDemuxedOnly; Ditto. Leave it to the sub-classes. ::: dom/media/MediaDecoderStateMachine.cpp @@ +553,5 @@ > (VideoQueue().IsFinished() || VideoQueue().GetSize() > 0)); > } > > void > +MediaDecoderStateMachine::OnAudioProcessed(MediaData* aAudioSample) The name is just too generic to mean anything to me. Though, I don't have a better name for now. I would prefer to just leave it as is until we come up with a better name. @@ +1898,5 @@ > mResource->SetReadMode(MediaCacheStream::MODE_PLAYBACK); > mDecoder->SetMediaSeekable(mReader->IsMediaSeekable()); > mInfo = aMetadata->mInfo; > + // Determine the state machine should request decoded or demuxed-only data. > + mIsExternalRenderingMode = mInfo.mAudio.mIsRenderedExternally || I can't see how using an external renderer is corelated to demux-only mode. In software engineering, it is a good practice to separate policy from mechanism. Adding demux-only mode to the reader is mechanism. Determining when to switch to demux-only mode is policy. You should have separate bugs to make your intention clear. @@ +1956,5 @@ > // to become available so that we can build the correct decryptor/decoder. > SetState(DECODER_STATE_WAIT_FOR_CDM); > return; > } > + if (mIsExternalRenderingMode) { Ditto. ::: dom/media/MediaFormatReader.h @@ +89,5 @@ > > bool IsWaitForDataSupported() override { return true; } > + > + // MediaFormatReader supports demuxed-only mode. > + bool IsDemuxOnlySupported() override { return true; } const. @@ +122,5 @@ > // Returns true if no more action is required. > bool UpdateReceivedNewData(TrackType aTrack); > // Called when new samples need to be demuxed. > void RequestDemuxSamples(TrackType aTrack); > + // Handle demuxed samples by the input behavior. Ask jya for review. I don't have a good understanding how this should be implemented in the reader.
Attachment #8659812 - Flags: feedback?(jwwang) → feedback-
Depends on: 1204799
Thanks for the feedback. I created the dependent Bug 1204799 for discussing how to request the sample from reader.
(In reply to JW Wang [:jwwang] from comment #13) > Comment on attachment 8659812 [details] [diff] [review] > v1-Make-MediaDecoderStateMachine-capable-of.patch > > Review of attachment 8659812 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderReader.h > @@ +167,5 @@ > > + // if data request is decoded or demuxed only. > > + void SetDemuxedOnly(bool aDemuxedOnly) { > > + mDemuxedOnly = true; > > + } > > + bool IsDemuxedOnly() { > > It doesn't make sense to return true if a reader doesn't support demux-only > mode. SetDemuxedOnly() and IsDemuxedOnly() should be empty here and leave > the implementation to sub-classes. > Addressed. Base class always return false, and derived class determines the logic. > @@ +437,5 @@ > > bool mHitAudioDecodeError; > > bool mShutdown; > > > > + // Set the demuxed-only flag. > > + bool mDemuxedOnly; > > Ditto. Leave it to the sub-classes. > Addressed. > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +553,5 @@ > > (VideoQueue().IsFinished() || VideoQueue().GetSize() > 0)); > > } > > > > void > > +MediaDecoderStateMachine::OnAudioProcessed(MediaData* aAudioSample) > > The name is just too generic to mean anything to me. Though, I don't have a > better name for now. I would prefer to just leave it as is until we come up > with a better name. > rollbacked. > @@ +1898,5 @@ > > mResource->SetReadMode(MediaCacheStream::MODE_PLAYBACK); > > mDecoder->SetMediaSeekable(mReader->IsMediaSeekable()); > > mInfo = aMetadata->mInfo; > > + // Determine the state machine should request decoded or demuxed-only data. > > + mIsExternalRenderingMode = mInfo.mAudio.mIsRenderedExternally || > > I can't see how using an external renderer is corelated to demux-only mode. > In software engineering, it is a good practice to separate policy from > mechanism. Adding demux-only mode to the reader is mechanism. Determining > when to switch to demux-only mode is policy. You should have separate bugs > to make your intention clear. > OK, moved out the logic for config the reader. Discuss in other thread Bug 1204799. > @@ +1956,5 @@ > > // to become available so that we can build the correct decryptor/decoder. > > SetState(DECODER_STATE_WAIT_FOR_CDM); > > return; > > } > > + if (mIsExternalRenderingMode) { > > Ditto. > > ::: dom/media/MediaFormatReader.h > @@ +89,5 @@ > > > > bool IsWaitForDataSupported() override { return true; } > > + > > + // MediaFormatReader supports demuxed-only mode. > > + bool IsDemuxOnlySupported() override { return true; } > > const. > Addressed > @@ +122,5 @@ > > // Returns true if no more action is required. > > bool UpdateReceivedNewData(TrackType aTrack); > > // Called when new samples need to be demuxed. > > void RequestDemuxSamples(TrackType aTrack); > > + // Handle demuxed samples by the input behavior. > > Ask jya for review. I don't have a good understanding how this should be > implemented in the reader. OK, I will request JYA for reviewing. Thank you very much.
Attachment #8659812 - Attachment is obsolete: true
Attachment #8661623 - Flags: feedback?(jwwang)
Comment on attachment 8661623 [details] [diff] [review] v2-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8661623 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.h @@ +1096,5 @@ > // being "prerolled". > bool mIsAudioPrerolling; > bool mIsVideoPrerolling; > + // Request demuxed data if this flag is true. > + bool mIsExternalRenderingMode; This should be left to bug 1204799. ::: dom/media/MediaFormatReader.cpp @@ +1241,5 @@ > > // Demux samples if we don't have some. > RequestDemuxSamples(aTrack); > + > + if (IsDemuxedOnly()) { It would be a data race if SetDemuxedOnly() is called by MDSM on the state machine thread. Do you really need to expose IsDemuxedOnly() from the reader? If IsDemuxOnlySupported() return true, we should be able to assume SetDemuxedOnly() will always succeed, right? ::: dom/media/MediaFormatReader.h @@ +91,5 @@ > + > + // MediaFormatReader supports demuxed-only mode. > + bool IsDemuxOnlySupported() const override { return true; } > + > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override space.
Attachment #8661623 - Flags: feedback?(jwwang) → feedback-
Hi JW, Thanks for the reviewing. I revised the patch as you mentioned. (In reply to JW Wang [:jwwang] from comment #16) > Comment on attachment 8661623 [details] [diff] [review] > v2-Make-MediaDecoderStateMachine-capable-of.patch > > Review of attachment 8661623 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderStateMachine.h > @@ +1096,5 @@ > > // being "prerolled". > > bool mIsAudioPrerolling; > > bool mIsVideoPrerolling; > > + // Request demuxed data if this flag is true. > > + bool mIsExternalRenderingMode; > > This should be left to bug 1204799. > OK, deleted. > ::: dom/media/MediaFormatReader.cpp > @@ +1241,5 @@ > > > > // Demux samples if we don't have some. > > RequestDemuxSamples(aTrack); > > + > > + if (IsDemuxedOnly()) { > > It would be a data race if SetDemuxedOnly() is called by MDSM on the state > machine thread. Do you really need to expose IsDemuxedOnly() from the > reader? If IsDemuxOnlySupported() return true, we should be able to assume > SetDemuxedOnly() will always succeed, right? > Yes, so I removed this API. Thanks for reminding. > ::: dom/media/MediaFormatReader.h > @@ +91,5 @@ > > + > > + // MediaFormatReader supports demuxed-only mode. > > + bool IsDemuxOnlySupported() const override { return true; } > > + > > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override > > space. Addressed.
Attachment #8661623 - Attachment is obsolete: true
Attachment #8662233 - Flags: feedback?(jwwang)
Comment on attachment 8662233 [details] [diff] [review] v3-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8662233 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderReader.h @@ +163,5 @@ > + // retuning demuxed data. > + virtual bool IsDemuxOnlySupported() const { return false; } > + > + // Let MediaDecoderStateMachine can configure > + // if data request is decoded or demuxed only. Suggested comment: Configure the reader to return demuxed or decoded data upon calls to Request{Audio,Video}Data. @@ +164,5 @@ > + virtual bool IsDemuxOnlySupported() const { return false; } > + > + // Let MediaDecoderStateMachine can configure > + // if data request is decoded or demuxed only. > + // Derived class should handle if caller wants to request the demuxed-only This comment is not necessary. Sub-classes should always follow the spec of the base class. ::: dom/media/MediaFormatReader.h @@ +91,5 @@ > + > + // MediaFormatReader supports demuxed-only mode. > + bool IsDemuxOnlySupported() const override { return true; } > + > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override The convention here is to omit the 'virtual' keyword. @@ +93,5 @@ > + bool IsDemuxOnlySupported() const override { return true; } > + > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override > + { > + mDemuxedOnly = aDemuxedOnly; You still didn't address the data race issue.
Attachment #8662233 - Flags: feedback?(jwwang) → feedback-
(In reply to JW Wang [:jwwang] from comment #18) > Comment on attachment 8662233 [details] [diff] [review] > v3-Make-MediaDecoderStateMachine-capable-of.patch > > Review of attachment 8662233 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderReader.h > @@ +163,5 @@ > > + // retuning demuxed data. > > + virtual bool IsDemuxOnlySupported() const { return false; } > > + > > + // Let MediaDecoderStateMachine can configure > > + // if data request is decoded or demuxed only. > > Suggested comment: Configure the reader to return demuxed or decoded data > upon calls to Request{Audio,Video}Data. > Followed. > @@ +164,5 @@ > > + virtual bool IsDemuxOnlySupported() const { return false; } > > + > > + // Let MediaDecoderStateMachine can configure > > + // if data request is decoded or demuxed only. > > + // Derived class should handle if caller wants to request the demuxed-only > > This comment is not necessary. Sub-classes should always follow the spec of > the base class. > Addressed. > ::: dom/media/MediaFormatReader.h > @@ +91,5 @@ > > + > > + // MediaFormatReader supports demuxed-only mode. > > + bool IsDemuxOnlySupported() const override { return true; } > > + > > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override > > The convention here is to omit the 'virtual' keyword. > Addressed. > @@ +93,5 @@ > > + bool IsDemuxOnlySupported() const override { return true; } > > + > > + virtual void SetDemuxedOnly(bool aDemuxedOnly) override > > + { > > + mDemuxedOnly = aDemuxedOnly; > > You still didn't address the data race issue. Sorry, Using Atomic<bool> instead.
Attachment #8662233 - Attachment is obsolete: true
Attachment #8662299 - Flags: feedback?(jwwang)
Comment on attachment 8662299 [details] [diff] [review] v4-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8662299 [details] [diff] [review]: ----------------------------------------------------------------- Leave the reader part to jya.
Attachment #8662299 - Flags: feedback?(jwwang) → feedback+
Comment on attachment 8662299 [details] [diff] [review] v4-Make-MediaDecoderStateMachine-capable-of.patch Hi JYA, Could you please help for reviewing this patch? If the reader supports demuxed-only mode. I'm not quite sure if my modification can work well when demuxing complete. I think it should callback to MDSM without doing the decoding. Please review this part, thank you very much.
Attachment #8662299 - Flags: review?(jyavenard)
Comment on attachment 8662299 [details] [diff] [review] v4-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8662299 [details] [diff] [review]: ----------------------------------------------------------------- There are several problems at hand: 1- You will create a video decoder for nothing ; would be good to have a way to let the reader decide at initialization's time if we need a decoder. 2- You have no need for using a fonction's pointer ; simply return the sample in place of feeding it the the decoder (and I'm no fan on function's pointer in C++), you're already performing this test anyway prior calling the function.
Attachment #8662299 - Flags: review?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #22) > Comment on attachment 8662299 [details] [diff] [review] > v4-Make-MediaDecoderStateMachine-capable-of.patch > > Review of attachment 8662299 [details] [diff] [review]: > ----------------------------------------------------------------- > > There are several problems at hand: > > 1- You will create a video decoder for nothing ; would be good to have a way > to let the reader decide at initialization's time if we need a decoder. As I know, we will instantiate the decoder when entering |MediaFormatReader::EnsureDecodersCreated()|. MDSM will get the mIsRenderedExternally flag when MetaData request is resolved by reader, then MDSM can configure the reader at this timing(Maybe need some logic discussed in Bug 1204799). 1. I'm not sure the order between "configuring reader" and "instantiate the decoder". If "configuring reader" can be done before "instantiate the decoder", maybe we can prevent the decoder from creating. 2. I'm not sure if we thoroughly don't need the decoder in demuxed-only scenario. I think we can revise this issue more comprehensively in Bug 1194652 - Make MediaFormatReader capable of returning only demuxed raw data for EME HW CDM Rendering Since there has no one config reader yet, this refactorying can be done in above bug I think... Does it make sense? > 2- You have no need for using a function's pointer ; simply return the > sample in place of feeding it the the decoder (and I'm no fan on function's > pointer in C++), you're already performing this test anyway prior calling > the function. Thank you, I removed the function pointer and determine return directly or decoded directly.
Attachment #8662299 - Attachment is obsolete: true
Attachment #8662772 - Flags: feedback?(jyavenard)
Comment on attachment 8662772 [details] [diff] [review] v5-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8662772 [details] [diff] [review]: ----------------------------------------------------------------- just realising that this is going to fail for MSE when changing resolution where we would normally perform an internal seek. Especially, when in some case we may have to go backward in the stream. Typically, when encountering a frame of a new resolution, it will be a keyframe : in which case you only need to return the frame and continue on like nothing happened. However, if somehow the sourcebuffer got modified concurrently (like the user force change the resolution), we have to go back to the previous keyframe , start decoding and drop all the frames until we reach the original decode time. I have no idea on how you could be handling this with an external decoder and pass all the information required to perform this task :( We will likely have to move this logic inside the MDSM. But as it is, it won't work sorry. ::: dom/media/MediaFormatReader.cpp @@ +990,5 @@ > + } else { > + mAudio.mPromise.Resolve(aSample, __func__); > + } > + return true; > +} this is not needed. Use ReturnOutput() instead. @@ +1100,5 @@ > + > + if (mDemuxedOnly) { > + if (!ReturnDemuxedSamples(aTrack, sample)) { > + NotifyError(aTrack); > + return; this is going to fail if you have multiple demuxed samples pending (and typically you will have more than one) You need to stop after the first sample. @@ +1103,5 @@ > + NotifyError(aTrack); > + return; > + } > + } else { > + if (!DecodeDemuxedSamples(aTrack, sample)) { else if { ::: dom/media/MediaFormatReader.h @@ +133,1 @@ > AbstractMediaDecoder::AutoNotifyDecoded& aA); you don't need this method. please use ReturnOutput(); so the discontinuity flag will be properly handled.
Attachment #8662772 - Flags: feedback?(jyavenard) → feedback-
(In reply to Jean-Yves Avenard [:jya] from comment #24) > Comment on attachment 8662772 [details] [diff] [review] > v5-Make-MediaDecoderStateMachine-capable-of.patch > > Review of attachment 8662772 [details] [diff] [review]: > ----------------------------------------------------------------- > > just realising that this is going to fail for MSE when changing resolution > where we would normally perform an internal seek. > Especially, when in some case we may have to go backward in the stream. > > Typically, when encountering a frame of a new resolution, it will be a > keyframe : in which case you only need to return the frame and continue on > like nothing happened. > > However, if somehow the sourcebuffer got modified concurrently (like the > user force change the resolution), we have to go back to the previous > keyframe , start decoding and drop all the frames until we reach the > original decode time. > > I have no idea on how you could be handling this with an external decoder > and pass all the information required to perform this task :( > > We will likely have to move this logic inside the MDSM. > > But as it is, it won't work sorry. > Thanks for pointing out this case, We will need to consider how much information the external decoder needs to know and maybe expose an API such as NotifyResolutionChanged(...) in GMP Renderer APIs on bug 1146796. > ::: dom/media/MediaFormatReader.cpp > @@ +990,5 @@ > > + } else { > > + mAudio.mPromise.Resolve(aSample, __func__); > > + } > > + return true; > > +} > > this is not needed. > Use ReturnOutput() instead. > > @@ +1100,5 @@ > > + > > + if (mDemuxedOnly) { > > + if (!ReturnDemuxedSamples(aTrack, sample)) { > > + NotifyError(aTrack); > > + return; > > this is going to fail if you have multiple demuxed samples pending (and > typically you will have more than one) > > You need to stop after the first sample. > > @@ +1103,5 @@ > > + NotifyError(aTrack); > > + return; > > + } > > + } else { > > + if (!DecodeDemuxedSamples(aTrack, sample)) { > > else if { > > ::: dom/media/MediaFormatReader.h > @@ +133,1 @@ > > AbstractMediaDecoder::AutoNotifyDecoded& aA); > > you don't need this method. > please use ReturnOutput(); so the discontinuity flag will be properly > handled. It seems there are still some logic needs to be changed. Could I removed all the request data logic out of this patch and continue Bug 1194652 to accomplish the remaining work? Thank you.
Flags: needinfo?(jyavenard)
(In reply to James Cheng[:JamesCheng] from comment #25) > Thanks for pointing out this case, > > We will need to consider how much information the external decoder needs to > know and maybe expose an API such as NotifyResolutionChanged(...) in GMP > Renderer APIs on bug 1146796. We could be done would be to have a NotifyContentChanged(int64_t aTime); (resolution is too much linked to video, and the same can happen for audio) that tells the decoder that a new stream content is coming ; The next sample to come is guaranteed to be a keyframe and that it needs to decode all frames and drop all those whose time is < aTime ; and to start to redisplay from aTime only. This is what the MediaFormatReader is doing internally. > > > ::: dom/media/MediaFormatReader.cpp > > @@ +990,5 @@ > > > + } else { > > > + mAudio.mPromise.Resolve(aSample, __func__); > > > + } > > > + return true; > > > +} > > > > this is not needed. > > Use ReturnOutput() instead. > > > > @@ +1100,5 @@ > > > + > > > + if (mDemuxedOnly) { > > > + if (!ReturnDemuxedSamples(aTrack, sample)) { > > > + NotifyError(aTrack); > > > + return; > > > > this is going to fail if you have multiple demuxed samples pending (and > > typically you will have more than one) > > > > You need to stop after the first sample. > > > > @@ +1103,5 @@ > > > + NotifyError(aTrack); > > > + return; > > > + } > > > + } else { > > > + if (!DecodeDemuxedSamples(aTrack, sample)) { > > > > else if { > > > > ::: dom/media/MediaFormatReader.h > > @@ +133,1 @@ > > > AbstractMediaDecoder::AutoNotifyDecoded& aA); > > > > you don't need this method. > > please use ReturnOutput(); so the discontinuity flag will be properly > > handled. > > > It seems there are still some logic needs to be changed. > > Could I removed all the request data logic out of this patch and continue > Bug 1194652 to accomplish the remaining work? > > Thank you. not sure I understand your concern here. Something like: if (mDemuxedOnly) { ReturnOutput(aTrack, sample); } else if (!DecodeDemuxedSamples(aTrack, sample)) { NotifyError(aTrack); return; } decoder.mQueuedSamples.RemoveElementAt(0); if (mDemuxedOnly) { return; } .... should do.
Flags: needinfo?(jyavenard)
Target Milestone: FxOS-S7 (18Sep) → FxOS-S8 (02Oct)
(In reply to Jean-Yves Avenard [:jya] from comment #26) > (In reply to James Cheng[:JamesCheng] from comment #25) > > > Thanks for pointing out this case, > > > > We will need to consider how much information the external decoder needs to > > know and maybe expose an API such as NotifyResolutionChanged(...) in GMP > > Renderer APIs on bug 1146796. > > We could be done would be to have a NotifyContentChanged(int64_t aTime); > (resolution is too much linked to video, and the same can happen for audio) > > that tells the decoder that a new stream content is coming ; The next sample > to come is guaranteed to be a keyframe and that it needs to decode all > frames and drop all those whose time is < aTime ; and to start to redisplay > from aTime only. > > This is what the MediaFormatReader is doing internally. > Thank you, I will add this API in GMP API(Bug 1146796). > > > > > ::: dom/media/MediaFormatReader.cpp > > > @@ +990,5 @@ > > > > + } else { > > > > + mAudio.mPromise.Resolve(aSample, __func__); > > > > + } > > > > + return true; > > > > +} > > > > > > this is not needed. > > > Use ReturnOutput() instead. OK! > > > > > > @@ +1100,5 @@ > > > > + > > > > + if (mDemuxedOnly) { > > > > + if (!ReturnDemuxedSamples(aTrack, sample)) { > > > > + NotifyError(aTrack); > > > > + return; > > > > > > this is going to fail if you have multiple demuxed samples pending (and > > > typically you will have more than one) > > > > > > You need to stop after the first sample. > > > > > > @@ +1103,5 @@ > > > > + NotifyError(aTrack); > > > > + return; > > > > + } > > > > + } else { > > > > + if (!DecodeDemuxedSamples(aTrack, sample)) { > > > > > > else if { > > > > > > ::: dom/media/MediaFormatReader.h > > > @@ +133,1 @@ > > > > AbstractMediaDecoder::AutoNotifyDecoded& aA); > > > > > > you don't need this method. > > > please use ReturnOutput(); so the discontinuity flag will be properly > > > handled. > > > > > > It seems there are still some logic needs to be changed. > > > > Could I removed all the request data logic out of this patch and continue > > Bug 1194652 to accomplish the remaining work? > > > > Thank you. > > not sure I understand your concern here. > Something like: > if (mDemuxedOnly) { > ReturnOutput(aTrack, sample); > } else if (!DecodeDemuxedSamples(aTrack, sample)) { > NotifyError(aTrack); > return; > } > decoder.mQueuedSamples.RemoveElementAt(0); > if (mDemuxedOnly) { > return; > } > .... > > should do. I'm sorry that I misunderstand before, Using |ReturnOutput| to resolve the promise and do once then return. In ReturnOutput, I will judge if this sample is MediaRawData to avoid wrong casting to AudioData. Could you please review for this modification? Thanks for your help!
Attachment #8662772 - Attachment is obsolete: true
Attachment #8663086 - Flags: review?(jyavenard)
Comment on attachment 8663086 [details] [diff] [review] v6-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8663086 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. However, I don't see the point of having this change in knowing that it won't work in the only case we care about: MSE + EME. Better get the GMP spec ready with a working prototype first and then complete the work on the MediaFormatReader
Attachment #8663086 - Flags: review?(jyavenard)
Thank you! I will start to write a test code for verifying this patch. Maybe a gtest which takes TestMP4Demuxer.cpp for reference.
Hi James, Could you provide update for the progress? Thanks!
Flags: needinfo?(jacheng)
Whiteboard: [ft:conndevices][partner-blocker]
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
After discussing with JYA and JW, I'm writing a gtest for testing the demuxed-only scenario. We would like to verify this patch even it is not being used currently. It is good if this patch can be landed before 10/16 but It's not a 2.5 feature blocker, in eng's point of view, we'd like to see this code landed asap for EME HW CDM rendering. Thank you.
Flags: needinfo?(jacheng)
rebase this patch.
Hi JYA, Per IRC discussion, I encountered some gtest errors https://treeherder.mozilla.org/#/jobs?repo=try&revision=efcf9961067b Due to https://dxr.mozilla.org/mozilla-central/rev/67adec79eb8a481d87721a10b2aa733020d326eb/dom/media/MediaFormatReader.cpp#406 return false with MIME type "audio/mp4a-latm" The gtest only passed on "OS X 10.7 opt" and on my local PC. I will try to enable the pref "media.fragmented-mp4.ffmpeg.enabled" on linux first. And debug for Windows, WARNING: NS_ENSURE_TRUE(SUCCEEDED(hr)) failed: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/media/platforms/wmf/MFTDecoder.cpp, line 39 WARNING: NS_ENSURE_TRUE(IsSupportedAudioMimeType(mInfo.mAudio.mMimeType)). Could you please take look of my gtest patch to see if there is anything I misused or ignored, thank you.
Attachment #8663086 - Attachment is obsolete: true
Attachment #8670715 - Flags: feedback?(jyavenard)
It appears that the various framework haven't been loaded into memory. We rely on the gfx to have done the job of loading some libraries.
Depends on: 1212795
I haven't update the code with jya's PDMFactory. Update what I've seen on treeherder, I added some logs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3af9535baa5a For Linux platform, sGMPDecoderEnabled ＝ false sUseBlankDecoder = false FFmpegRuntimeLinker::CreateDecoderModule() will return null so we create CreateAgnosticDecoderModule, so the SupportsMimeType always returns false. Set |media.fragmented-mp4.ffmpeg.enabled |to true is also get the same fail result. For Windows platform, sGMPDecoderEnabled ＝ false sUseBlankDecoder = false WMFDecoderModule::SupportsMimeType will try to create WMFDecoder and the https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/MFTDecoder.cpp?from=MFTDecoder.cpp#39 returns hr = 0x80040154(REGDB_E_CLASSNOTREG), so SupportsMimeType returns false. I don't know why the I cannot get the COM component. Hi Jya, Can I try to set the pref |media.fragmented-mp4.use-blank-decoder| to true using the BlankDecoder for all the platform? Since I have no idea why I always encounter those errors(Maybe there is something I ignore to init on my gtest). Thanks.
I indicated on IRC why you had the problem with ffmpeg on linux and the try machine. We don't use the libav 0.8.5 found on try by default. For the purpose of your gtest, and seeing that you have no use for the decoder anyway, I don't see why you couldn't use the blank decoder in your gtest
I ran the patch attached in a Windows 10 VM, and your tests pass. Looking into the code, other than for linux (which won't work for the reason explained above), there doesn't appear to be anything wrong. The reasons for disabling libav 0.8 and earlier from being used was purely to prevent ffmpeg to be used on try. But this is no longer the case. I'll open a patch to remove this restriction; you can apply that patch to fix your problem.
Depends on: 1213173
Depends on: 1213176
Attachment #8670709 - Attachment is obsolete: true
Attachment #8672562 - Flags: review?(jyavenard)
Hi JYA, I used BlankDecoder for my test and it works fine on linux and windows. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b809c034c0fd Could you please review these two patches for enabling MediaFormatReader to support demuxed-only scenario and a gtest to verify the scenario? Thank you for your help.
Attachment #8670715 - Attachment is obsolete: true
Attachment #8670715 - Flags: feedback?(jyavenard)
Attachment #8672564 - Flags: review?(jyavenard)
Comment on attachment 8672562 [details] [diff] [review] Part1-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8672562 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. More importantly, I'd like to get a clear picture on what your threading model will be and where SetDemuxOnly will be called from ; we've worked hard to remove cross-threadings and it appears that you're about to add a new one ::: dom/media/MediaDecoderReader.h @@ +157,5 @@ > + virtual bool IsDemuxOnlySupported() const { return false; } > + > + // Configure the reader to return demuxed or decoded data > + // upon calls to Request{Audio,Video}Data. > + virtual void SetDemuxedOnly(bool /*aDemuxedOnly*/) {} SetDemuxOnly (so it's consistent with IsDemuxOnly) ::: dom/media/MediaFormatReader.cpp @@ +73,5 @@ > , mSeekable(false) > , mIsEncrypted(false) > , mTrackDemuxersMayBlock(false) > , mHardwareAccelerationDisabled(false) > + , mDemuxedOnly(false) mDemuxOnly @@ +902,3 @@ > MediaFormatReader::DecodeDemuxedSamples(TrackType aTrack, > + MediaRawData* aSample) > +{ MOZ_ASSERT(OnTaskQueue()) @@ +1025,3 @@ > decoder.mQueuedSamples.RemoveElementAt(0); > + if (mDemuxedOnly) { > + // If demuxed-only case, returnoutput will resolve with one demuxed data. ReturnOutput @@ +1191,5 @@ > + mInfo.mAudio.mChannels = audioData->mChannels; > + } > + mAudio.mPromise.Resolve(audioData, __func__); > + } else { > + mAudio.mPromise.Resolve(aData, __func__); seeing that mAudio is expecting a MediaData* you can simply return the original aData and remove the else { } ::: dom/media/MediaFormatReader.h @@ +91,5 @@ > + bool IsDemuxOnlySupported() const override { return true; } > + > + void SetDemuxedOnly(bool aDemuxedOnly) override > + { > + mDemuxedOnly = aDemuxedOnly; I'm unclear on your threading model. Who would call SetDemuxOnly() ? and on which thread? can't this be made to only ever be called on the reader's taskqueue ? We've managed to remove all cross-threading, and it seems you're about to add one.
Attachment #8672562 - Flags: review?(jyavenard)
Attachment #8672562 - Flags: review+
Attachment #8672562 - Flags: feedback?(jwwang)
Attachment #8672564 - Flags: review?(jyavenard) → review+
Comment on attachment 8672562 [details] [diff] [review] Part1-Make-MediaDecoderStateMachine-capable-of.patch Review of attachment 8672562 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +671,5 @@ > // to reach playing. > aSample->As<VideoData>()->mFrameID = ++mCurrentFrameID; > VideoQueue().Push(aSample); > } else { > + // Handle MediaRawData, Save these changes until MDSM is capable of receiving demuxed data. Related changes should be put together. @@ +694,5 @@ > } else if (aSample->mType == MediaData::VIDEO_DATA) { > aSample->As<VideoData>()->mFrameID = ++mCurrentFrameID; > VideoQueue().PushFront(aSample); > } else { > + // Handle MediaRawData, Ditto. ::: dom/media/MediaDecoderStateMachine.h @@ +357,5 @@ > // be held. > bool IsPlaying() const; > > + // TODO: Those callback function may receive demuxed-only data. > + // Need to figure out a suitable API name for this case. Will you do it in this bug or a new bug? ::: dom/media/MediaFormatReader.cpp @@ +1017,5 @@ > + > + if (mDemuxedOnly) { > + ReturnOutput(sample, aTrack); > + } else if (!DecodeDemuxedSamples(aTrack, sample)) { > + NotifyError(aTrack); indention.
Attachment #8672562 - Flags: feedback?(jwwang) → feedback+
Comment on attachment 8672564 [details] [diff] [review] Part2-Add-gtest-for-demuxed-only-scenario-in-M.patch Review of attachment 8672564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gtest/TestMediaFormatReader.cpp @@ +23,5 @@ > + // Mock this method to return a imagecontainer for video case. > + layers::ImageContainer* > + GetImageContainer() override > + { > + return new layers::ImageContainer(); It is bad to create a new object each time this function is called. @@ +47,5 @@ > + , mResource(new MockMediaResource(aFileName)) > + , mReader(new MediaFormatReader(mDecoder, new MP4Demuxer(mResource))) > + , mTaskQueue(new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK))) > + { > + NS_ProcessNextEvent(); What is this for? @@ +63,5 @@ > + } > + template<class Function> > + void RunTestAndWait(Function aFunction) > + { > + nsRefPtr<nsRunnable> r = NS_NewRunnableFunction(Move(aFunction)); It is clumsy to copy and then move. Perfect forwarding will do better. @@ +124,5 @@ > +template <> > +void SetPref<float>(const char* aPrefKey, float aValue) > +{ > + unused << Preferences::SetFloat(aPrefKey, aValue); > +} It looks weird for most specializations are not used at all. You should just provide the one you need or extract it to a utility class to be reusable for other test cases. @@ +127,5 @@ > + unused << Preferences::SetFloat(aPrefKey, aValue); > +} > + > +template <typename T> > +class PreferencesRAII Why do we need an RAII class for pref? @@ +155,5 @@ > + b->Init(); > + EXPECT_TRUE(b->mReader->IsDemuxOnlySupported()); > + // Switch to demuxed-only mode. > + b->mReader->SetDemuxedOnly(true); > + auto testCase = [b]() { Deep nesting code is hard to read. Try to flatten the hierarchy.
Hi JYA and JW, > ::: dom/media/MediaFormatReader.h > @@ +91,5 @@ > > + bool IsDemuxOnlySupported() const override { return true; } > > + > > + void SetDemuxedOnly(bool aDemuxedOnly) override > > + { > > + mDemuxedOnly = aDemuxedOnly; > > I'm unclear on your threading model. > Who would call SetDemuxOnly() ? and on which thread? can't this be made to > only ever be called on the reader's taskqueue ? > > We've managed to remove all cross-threading, and it seems you're about to > add one. Thanks for your reminding, I will dispatch to its thread to prevent from the crossing thread access. > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +671,5 @@ > > // to reach playing. > > aSample->As<VideoData>()->mFrameID = ++mCurrentFrameID; > > VideoQueue().Push(aSample); > > } else { > > + // Handle MediaRawData, > > Save these changes until MDSM is capable of receiving demuxed data. Related > changes should be put together. OK, I will remove the changes. > ::: dom/media/MediaDecoderStateMachine.h > @@ +357,5 @@ > > // be held. > > bool IsPlaying() const; > > > > + // TODO: Those callback function may receive demuxed-only data. > > + // Need to figure out a suitable API name for this case. > > Will you do it in this bug or a new bug? > do it in a new bug when this patch is landed. > ::: dom/media/gtest/TestMediaFormatReader.cpp > @@ +23,5 @@ > > + // Mock this method to return a imagecontainer for video case. > > + layers::ImageContainer* > > + GetImageContainer() override > > + { > > + return new layers::ImageContainer(); > > It is bad to create a new object each time this function is called. I will remove this method based on Bug 1213176. No need to mock this method. > @@ +47,5 @@ > > + , mResource(new MockMediaResource(aFileName)) > > + , mReader(new MediaFormatReader(mDecoder, new MP4Demuxer(mResource))) > > + , mTaskQueue(new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK))) > > + { > > + NS_ProcessNextEvent(); > > What is this for? https://dxr.mozilla.org/mozilla-central/rev/11ff0ccb7d59311df4c190d331c8b58c6e35a0c8/dom/media/MediaDecoderReader.cpp#86 To ensure this task can be done before other tasks. > > + template<class Function> > > + void RunTestAndWait(Function aFunction) > > + { > > + nsRefPtr<nsRunnable> r = NS_NewRunnableFunction(Move(aFunction)); > > It is clumsy to copy and then move. Perfect forwarding will do better. OK, Thanks. > @@ +124,5 @@ > > +template <> > > +void SetPref<float>(const char* aPrefKey, float aValue) > > +{ > > + unused << Preferences::SetFloat(aPrefKey, aValue); > > +} > > It looks weird for most specializations are not used at all. You should just > provide the one you need or extract it to a utility class to be reusable for > other test cases. > I will removed the unused specializations. Thanks > @@ +127,5 @@ > > + unused << Preferences::SetFloat(aPrefKey, aValue); > > +} > > + > > +template <typename T> > > +class PreferencesRAII > > Why do we need an RAII class for pref? I want to recover the initial value when my gtest finished. I don't want to affect other test case. > @@ +155,5 @@ > > + b->Init(); > > + EXPECT_TRUE(b->mReader->IsDemuxOnlySupported()); > > + // Switch to demuxed-only mode. > > + b->mReader->SetDemuxedOnly(true); > > + auto testCase = [b]() { > > Deep nesting code is hard to read. Try to flatten the hierarchy. OK, I will revise it into different functions.
Rebase and fix nit and the review feedback.
Attachment #8672562 - Attachment is obsolete: true
Attachment #8673522 - Flags: review+
Attachment #8673522 - Flags: feedback+
Fix all the things on comment 43. Hi JW, Please take a look with this patch. I remove the nested code with functions. Refer to Bug 1214073, I modified the function into void SetDemuxOnly(bool aDemuxedOnly) override { if (OnTaskQueue()) { mDemuxOnly = aDemuxedOnly; return; } nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethodWithArg<bool>( this, &MediaDecoderReader::SetDemuxOnly, aDemuxedOnly); OwnerThread()->Dispatch(r.forget()); } to avoid cross thread accessing. Thanks.
Attachment #8672564 - Attachment is obsolete: true
Attachment #8673525 - Flags: review+
Attachment #8673525 - Flags: feedback?(jwwang)
Attachment #8673525 - Attachment is obsolete: true
Attachment #8673525 - Flags: feedback?(jwwang)
Attachment #8673527 - Flags: review+
Attachment #8673527 - Flags: feedback?(jwwang)
Comment on attachment 8673527 [details] [diff] [review] Part2-Add-gtest-for-demuxed-only-scenario-in-M.patch Review of attachment 8673527 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gtest/TestMediaFormatReader.cpp @@ +128,5 @@ > + } > +private: > + ~MediaFormatReaderBinding() > + { > + mDecoder = nullptr; It is pointless to reset these smart pointers in the destructor. @@ +175,5 @@ > +}; > + > +TEST(MediaFormatReader, RequestAudioRawData) > +{ > + PreferencesRAII<bool> pref = Just say |pref("media.fragmented-mp4.use-blank-decoder", true)| so you construct the object directly without copy-construct from a temp. @@ +203,5 @@ > + PreferencesRAII<bool> pref = > + PreferencesRAII<bool>("media.fragmented-mp4.use-blank-decoder", > + true); > + nsRefPtr<MediaFormatReaderBinding> b = new MediaFormatReaderBinding(); > + b->Init(); Have Init() return a value so you won't continue the test when it fails. @@ +204,5 @@ > + PreferencesRAII<bool>("media.fragmented-mp4.use-blank-decoder", > + true); > + nsRefPtr<MediaFormatReaderBinding> b = new MediaFormatReaderBinding(); > + b->Init(); > + EXPECT_TRUE(b->mReader->IsDemuxOnlySupported()); The test can't continue if the reader doesn't support demux-only mode. However, it would be awkward to choose such a reader to begin with. @@ +208,5 @@ > + EXPECT_TRUE(b->mReader->IsDemuxOnlySupported()); > + // Switch to demuxed-only mode. > + b->mReader->SetDemuxOnly(true); > + // To ensure the MediaDecoderReader::InitializationTask and > + // MediaDecoderReader::SetDemuxOnly ancan be done. "can be done".
Attachment #8673527 - Flags: feedback?(jwwang) → feedback+
Carry r+ and f+ and fix the review comment. > @@ +175,5 @@ > > +}; > > + > > +TEST(MediaFormatReader, RequestAudioRawData) > > +{ > > + PreferencesRAII<bool> pref = > > Just say |pref("media.fragmented-mp4.use-blank-decoder", true)| so you > construct the object directly without copy-construct from a temp. Actually it is a legal semantic to initial an object though it looks like doing the copy from a temp object(indeed, it only invoke the constructor without copy). And mozilla guideline indicated |In general, initialize variables with nsFoo aFoo = bFoo and not nsFoo aFoo(bFoo).| So I decided to obey. Attach the treeherder result https://treeherder.mozilla.org/#/jobs?repo=try&revision=662e745c66ee Thanks for your reviewing!
Attachment #8673527 - Attachment is obsolete: true
Attachment #8673769 - Flags: review+
Attachment #8673769 - Flags: feedback+
Keywords: checkin-needed
this failed to apply : applying 0001-Bug-1194606-Make-MediaDecoderStateMachine-capable-of.patch patching file dom/media/MediaFormatReader.h Hunk #1 FAILED at 78 1 out of 3 hunks FAILED -- saving rejects to file dom/media/MediaFormatReader.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 0001-Bug-1194606-Make-MediaDecoderStateMachine-capable-of.patch could you take a look, thanks!
Flags: needinfo?(jacheng)
Keywords: checkin-needed
this bug depened on bug 1213176...
Flags: needinfo?(jacheng)
Rebase for the hg apply rejection. Hi Jya, Sorry that I ignore this... If your patch landed, I don't need to mock |layers::ImageContainer* GetImageContainer() override|. I will request to check-in this code after this bug landed. Thank you.
Attachment #8673522 - Attachment is obsolete: true
Attachment #8674083 - Flags: review+
Attachment #8674083 - Flags: feedback+
I've finished my test with it simply waiting on try to complete (I can't locally build now as my computer SSD is dead waiting for a replacement)
Rebase the patch.
Attachment #8674083 - Attachment is obsolete: true
Attachment #8675305 - Flags: review+
Attachment #8675305 - Flags: feedback+
Rebase the patch. Attach the treeherder result for reference. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4847fdb20dcc
Attachment #8673769 - Attachment is obsolete: true
Attachment #8675306 - Flags: review+
Attachment #8675306 - Flags: feedback+
Keywords: checkin-needed
Hi, this maybe need a rebase again or ? patching file dom/media/MediaDecoderReader.h Hunk #1 FAILED at 166 1 out of 1 hunks FAILED -- saving rejects to file dom/media/MediaDecoderReader.h.rej patching file dom/media/MediaFormatReader.cpp Hunk #2 FAILED at 565 1 out of 5 hunks FAILED -- saving rejects to file dom/media/MediaFormatReader.cpp.rej patching file dom/media/MediaFormatReader.h Hunk #1 FAILED at 76 1 out of 3 hunks FAILED -- saving rejects to file dom/media/MediaFormatReader.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 0001-Bug-1194606-Make-MediaDecoderStateMachine-capable-of.patch
Flags: needinfo?(jacheng)
Keywords: checkin-needed
Updated for nsRefPtr to RefPtr.
Attachment #8675305 - Attachment is obsolete: true
Flags: needinfo?(jacheng)
Attachment #8675529 - Flags: review+
Attachment #8675529 - Flags: feedback+
Updated for nsRefPtr to RefPtr.
Attachment #8675306 - Attachment is obsolete: true
Attachment #8675530 - Flags: review+
Attachment #8675530 - Flags: feedback+
Keywords: checkin-needed
Hi, it still failed to apply: patching file dom/media/MediaDecoderReader.h Hunk #1 FAILED at 168 1 out of 1 hunks FAILED -- saving rejects to file dom/media/MediaDecoderReader.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh 0001-Bug-1194606-Make-MediaDecoderStateMachine-capable-of.patch maybe need another rebase ?
Flags: needinfo?(jacheng)
Rebase for applying patch failed.
Attachment #8675529 - Attachment is obsolete: true
Attachment #8676208 - Flags: review+
Attachment #8676208 - Flags: feedback+
Rebase for applying patch failed. Hi Carsten, I can successfully rebase for the current patch without any audo-merge logs. Could you please try again? Once it still has errors, could you please attach the .rej for me? Thank you.
Attachment #8675530 - Attachment is obsolete: true
Flags: needinfo?(jacheng)
Attachment #8676210 - Flags: review+
Attachment #8676210 - Flags: feedback+
Keywords: checkin-needed
Find build failed when updating to latest codebase. Fixing...
Rebase for the change of current codebase.
Attachment #8676210 - Attachment is obsolete: true
Attachment #8676375 - Flags: review+
Attachment #8676375 - Flags: feedback+
Keywords: checkin-needed
Attached file MediaFormatReader.h.rej (obsolete) —
Attached file MediaDecoderReader.h.rej (obsolete) —
Attachment #8676836 - Attachment mime type: application/x-reject → text/plain
Attachment #8676837 - Attachment mime type: application/x-reject → text/plain
attached the 2 rej files
Flags: needinfo?(jacheng)
Hi Carsten, Sorry that I am not familiar with the release process. I want to make sure the procedure that you apply the patch on |MC-Inbound| first? Once the inbound can be applied, you will merge into |MC-Master| right? It means I should make sure I can apply the patch into inbound since I can successfully apply the patch into master, I don't know why you met the failed case.... Thank you.
Flags: needinfo?(jacheng) → needinfo?(cbook)
(In reply to James Cheng[:JamesCheng] from comment #66) > Hi Carsten, > > Sorry that I am not familiar with the release process. > > I want to make sure the procedure that you apply the patch on |MC-Inbound| > first? > > Once the inbound can be applied, you will merge into |MC-Master| right? Hey James, yeah once a patch is on inbound and its not get backed out for test failures or so it will be merged by me or another sheriff to mozilla-central :) yes so yeah when you can push this to mozilla-inbound its fine :)
Flags: needinfo?(cbook)
Attachment #8676836 - Attachment is obsolete: true
Attachment #8676837 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: