Closed Bug 1319987 Opened 8 years ago Closed 8 years ago

Have MediaDataDecoder API use promises rather than callbacks

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 2 open bugs)

Details

Attachments

(12 files)

Bug 1319987: P1. Remove handling for WaitingForKey in MFR.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1319987: P2. Remove FuzzingWrapper.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P3. Remove Gonk PDM.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1319987: P4. Refactor H264 Converter.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1319987: P5. Promisify MediaDataDecoder.
59 bytes, text/x-review-board-request
cpearce
: review+
mozbugz
: review+
mattwoodrow
: review+
snorp
: review+
Details
Bug 1319987: P6. Shutdown demuxer asynchronously.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P7. Re-implement handling for WaitingForKey in MFR.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1319987: P8. Fix comment.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P9. More coding style fixes.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P10. Update WPT expected results.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P11. Fix MediaDataDecoder gtest.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1319987: P12. Disable RemoteDataDecoder. ?jhlin
59 bytes, text/x-review-board-request
jya
: review+
Details
This would simplify the design and the handling of the MediaFormatReader. It would also help with bug 944117, where we could use completion promises to chain the decoding of the normal plane and the alpha one.
Blocks: 1324937
Assignee: nobody → jyavenard
Priority: -- → P3
Comment on attachment 8828949 [details] Bug 1319987: P2. Remove FuzzingWrapper. https://reviewboard.mozilla.org/r/106174/#review107244 Bye bye little fuzzer...
Attachment #8828949 - Flags: review?(gsquelart) → review+
Depends on: 1332785
Comment on attachment 8828949 [details] Bug 1319987: P2. Remove FuzzingWrapper. https://reviewboard.mozilla.org/r/106174/#review107244 nah... I have an idea on how to add it back rather easily... at the time I just couldn't be bothered.
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review107346 ::: dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp:334 (Diff revision 1) > already_AddRefed<MediaDataDecoder> > EMEDecoderModule::CreateAudioDecoder(const CreateDecoderParams& aParams) > { > MOZ_ASSERT(aParams.mConfig.mCrypto.mValid); > > +<<<<<<< HEAD Merge conflict marker.
Comment on attachment 8828948 [details] Bug 1319987: P1. Remove handling for WaitingForKey in MFR. https://reviewboard.mozilla.org/r/106172/#review107358
Attachment #8828948 - Flags: review?(cpearce) → review+
Comment on attachment 8828951 [details] Bug 1319987: P4. Refactor H264 Converter. https://reviewboard.mozilla.org/r/106178/#review107364
Attachment #8828951 - Flags: review?(cpearce) → review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review107346 > Merge conflict marker. It was WIP, to show :gerald a problem with MozPromise preventing compilation of this code. See bug 1332785
Blocks: 1334061
Will remove the EME code for now in both MediaCodecDataDecoder and RemoteDataDecoder ; adapting those two to work with the new promise systems would require to duplicate hundred of lines of code, when all of those could be using a single MediaDataDecoderProxy instead to handle encrypted content. this will be continued in bug 1334061 instead
Comment on attachment 8828950 [details] Bug 1319987: P3. Remove Gonk PDM. https://reviewboard.mozilla.org/r/106176/#review108756 Assuming the OMX PDM is what is being used by the guys making HW acceleration work on the rasbperry pi (bug 1306529), then yes, we can remove the Gonk PDM.
Attachment #8828950 - Flags: review?(cpearce) → review+
Comment on attachment 8830973 [details] Bug 1319987: P6. Shutdown demuxer asynchronously. https://reviewboard.mozilla.org/r/107624/#review108770 Nice! r+ assuming the following questions don't raise anything bad: ::: dom/media/MediaFormatReader.cpp:487 (Diff revision 1) > ~DemuxerProxy() > { > MOZ_COUNT_DTOR(DemuxerProxy); > + } Are you sure you don't need to shutdown (if needed) from the destructor? ::: dom/media/MediaFormatReader.cpp:496 (Diff revision 1) > + > + RefPtr<ShutdownPromise> Shutdown() > + { > mData->mAudioDemuxer = nullptr; > mData->mVideoDemuxer = nullptr; > - RefPtr<Data> data = mData.forget(); > + RefPtr<Data> data = mData; I see you have removed `.forget()`, meaning you're still holding onto the mData from this MFR -- Is this intended?
Attachment #8830973 - Flags: review?(gsquelart) → review+
Comment on attachment 8830973 [details] Bug 1319987: P6. Shutdown demuxer asynchronously. https://reviewboard.mozilla.org/r/107624/#review108780 ::: dom/media/MediaFormatReader.cpp:487 (Diff revision 1) > ~DemuxerProxy() > { > MOZ_COUNT_DTOR(DemuxerProxy); > + } DemuxerProxy::Shutdown is always called from MediaFormatReader::Shutdown() ::: dom/media/MediaFormatReader.cpp:496 (Diff revision 1) > + > + RefPtr<ShutdownPromise> Shutdown() > + { > mData->mAudioDemuxer = nullptr; > mData->mVideoDemuxer = nullptr; > - RefPtr<Data> data = mData.forget(); > + RefPtr<Data> data = mData; it doesn't really matter here, because right after Shutdown() completes, the DemuxerProxy goes out of scope. But yes, I can keep the forget...
While you're obviously welcome to review everything, my r? request were for the following parts: cpearce: dom/media/gmp dom/media/platforms/agnostic/gmp dom/media/platforms/agnostic/eme mattwoodrow: dom/media/ipc snorp: dom/media/platforms/android gerald: everything :) With Chinese new year on, dom/media/platforms/omx is a problem, but Alfredo is no longer working on this, so he may not be able to review it regardless.
Flags: needinfo?(snorp)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gsquelart)
Flags: needinfo?(cpearce)
Flags: needinfo?(ayang)
How are you notifying the MDSM that it's waiting for key? Maybe I missed it, but I can't see how you're doing it. We're required by the EME spec to notify when we're waiting for key.
Flags: needinfo?(cpearce) → needinfo?(jyavenard)
(In reply to Chris Pearce (:cpearce) from comment #34) > How are you notifying the MDSM that it's waiting for key? Maybe I missed it, > but I can't see how you're doing it. We're required by the EME spec to > notify when we're waiting for key. Given that test_eme_waitingforkey fails, I'll assume you've not had a chance to get to this.
The relevant section of the EME spec is here: https://w3c.github.io/encrypted-media/#attempt-to-decrypt Basically, if we don't have a CDM, of if the CDM doesn't have a key we need, we're supposed to fire the event. Currently, we depend on the CDMProxy being instantiated for this to work; we only fire "waitingforkey" if the MediaKeys is set on the HTMLMediaElement. Bug 1306535 is on file to fix that. So it would be good if your solution here would also fix Bug 1306535. That is, what we do here should work regardless of whether a CDMProxy has been set on the MediaDecoder/MediaFormatReader or not. Given that we need to handle this in the case when we don't have a CDMProxy yet, can you handle this in the MediaFormatReader? On input, can you check whether the sample is encrypted, and if it is encrypted and we don't have a CDMProxy yet, fire waitingforkey. Otherwise when we do have a CDMProxy, poll the CDMProxy to see if that key is usable. You'll need to change the MediaFormatReader so that it gets notified when the key becomes usable. You'll also need to retry if a CDMProxy is set on the MFR. There may be a cleaner way to handle this that you can think of...
It's coming in P7.
Flags: needinfo?(jyavenard)
I'm not sure I follow just reading your description. As it currently is, having a CDMProxy is used to know when to use the EME PDM which is the only one using the SamplesWaitinfForKey. It is the SamplesWaitingForKey that would tell whatever is down the line to the MFR. So no CDMProxy, no EME, no SamplesWaitingForKey and as such no event when a key is missing. Not via the MFR that is. My plan in P7 is to restore the functionality removed in P1. I'll read the other bug to see how we can leverage the new architecture for this
Blocks: 1306535
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review108940 Eugen and John know this better than me at this point, but Android bits look fine.
Attachment #8828952 - Flags: review?(snorp) → review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109158 ::: dom/media/ipc/VideoDecoderParent.cpp:206 (Diff revision 5) > { > MOZ_ASSERT(!mDestroyed); > MOZ_ASSERT(OnManagerThread()); > - mDecoder->Drain(); > + RefPtr<VideoDecoderParent> self = this; > + mDecoder->Drain()->Then(mManagerTaskQueue, __func__, > + [self, this]() { Isn't this a DecodePromise, where the resolve function takes a list of decoded samples? Does MozPromise let you define lambdas that don't actually take the parameters needed? More importantly, don't we need to actually take those samples and send them back to the child? I think we want to change PVideoDecoder.ipdl to more closely match the new MediaDataDecoder definition. We can replace 'Output' and 'InputExhausted' with 'ResolveDecodePromise' and 'RejectDecodePromise' (with arguments that match the real promise) and call these from both Init and Drain.
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109160 r+ with typo, style, and optimization nits. I would recommend you get someone with proper working experience in the whole stack to have a look at the whole logic, e.g. JW? ::: dom/media/Benchmark.cpp:191 (Diff revision 6) > promise->Then( > Thread(), __func__, > [this, ref](RefPtr<MediaTrackDemuxer::SamplesHolder> aHolder) { > mSamples.AppendElements(Move(aHolder->mSamples)); > - if (ref->mParameters.mStopAtFrame && > - mSamples.Length() == (size_t)ref->mParameters.mStopAtFrame.ref()) { > + if (ref->mParameters.mStopAtFrame > + && mSamples.Length() == (size_t)ref->mParameters.mStopAtFrame.ref()) { While you're fixing the style, could you please change the C-style cast into C++? ::: dom/media/Benchmark.cpp:250 (Diff revision 6) > - } > - > + [ref, this]() { > + mDecoder->Shutdown()->Then( > + Thread(), __func__, > + [ref, this]() { > - mDecoderTaskQueue->BeginShutdown(); > + mDecoderTaskQueue->BeginShutdown(); > - mDecoderTaskQueue->AwaitShutdownAndIdle(); > + mDecoderTaskQueue->AwaitShutdownAndIdle(); It'd be great to make this promise-based too, instead of blocking! (Probably for another bug...) ::: dom/media/Benchmark.cpp:259 (Diff revision 6) > - Thread()->AsTaskQueue()->BeginShutdown()->Then( > + Thread()->AsTaskQueue()->BeginShutdown()->Then( > - ref->Thread(), __func__, > - [ref]() { ref->Dispose(); }, > + ref->Thread(), __func__, [ref]() { ref->Dispose(); }, > + []() { MOZ_CRASH("not reached"); }); Style: I'm guessing your magic clang formatter made you do this, but it'd look much more readable if the two lambdas were on seperate lines and aligned. ::: dom/media/Benchmark.cpp:282 (Diff revision 6) > - (frames == ref->mParameters.mFramesToMeasure || > - elapsedTime >= ref->mParameters.mTimeout)) { > + && (frames == ref->mParameters.mFramesToMeasure > + || elapsedTime >= ref->mParameters.mTimeout || mDrained)) { Style: If you start breaking lines in repeated-operator expression, I think each operand should be on a separate line. I.e.: ``` && (A || B || C) ``` Otherwise at a glance it looks like there are only two arguments. (If that's automated, fix the formatter, or fix its shoddy work before saving!) ::: dom/media/MediaFormatReader.h:203 (Diff revision 6) > void ShutdownDecoder() > { That's quite a bit of non-generic and non-critical code in a header, could you move it to the cpp file? ::: dom/media/MediaFormatReader.h:208 (Diff revision 6) > + TrackType type = mType == MediaData::AUDIO_DATA > + ? TrackType::kAudioTrack > + : TrackType::kVideoTrack; Style: Align '?' and ':' with 'mType'. ::: dom/media/MediaFormatReader.h:326 (Diff revision 6) > void Flush() > { That's quite a lot of non-generic and non-critical code in a header, could you move it to the cpp file? ::: dom/media/MediaFormatReader.cpp:243 (Diff revision 6) > ~Data() > { > - mTokenPromise.DisconnectIfExists(); > - mInitPromise.DisconnectIfExists(); > + mTokenRequest.DisconnectIfExists(); > + mInitRequest.DisconnectIfExists(); I noticed that you do these two DisconnectIfExists() in ShutdownDecoder() above, so do you still need them here? ::: dom/media/MediaFormatReader.cpp:893 (Diff revision 6) > + if (promises.IsEmpty()) { > + TearDownDecoders(); > + return MediaDecoderReader::Shutdown(); No need to set mShutdown to true before this return? (as you've done a bit below) ::: dom/media/MediaFormatReader.cpp:916 (Diff revision 6) > +{ > + LOGV("%s", TrackTypeToStr(aTrack)); > + > + auto& decoder = GetDecoderData(aTrack); > + if (!decoder.mFlushed && decoder.mDecoder) { > + // The decoder has yet to be flushed yet. Typo: Remove second 'yet'. ::: dom/media/MediaFormatReader.cpp:917 (Diff revision 6) > + LOGV("%s", TrackTypeToStr(aTrack)); > + > + auto& decoder = GetDecoderData(aTrack); > + if (!decoder.mFlushed && decoder.mDecoder) { > + // The decoder has yet to be flushed yet. > + // We always flush the decoder prior a shutdown to ensure that all the Typo: '... prior to a ...' ::: dom/media/MediaFormatReader.cpp:933 (Diff revision 6) > + > + if (!decoder.mDecoder) { > + // Shutdown any decoders that may be in the process of being initialized > + // in the Decoder Factory. > + // This will be a no-op until we're processing the final decoder shutdown > + // prior the MediaFormatReader being shutdown. Typo: 'prior to the ...' ::: dom/media/MediaFormatReader.cpp:955 (Diff revision 6) > + } > + if (!decoder.mShutdownPromise.IsEmpty()) { > + LOGV("Shutdown already in progress"); > + return; > + } > + ShutdownDecoderWithPromise(aTrack); It's the only time you are ignoring the returned promise from ShutdownDecoderWithPromise(). I would suggest you add an 'Unused <<' to show that that's intended. ::: dom/media/MediaFormatReader.cpp:1683 (Diff revision 6) > + [self, this, aTrack, > + &decoder](const MediaDataDecoder::DecodedData& aResults) { Style: That's a strange way to cut things! I'd suggest to have the capture on one line and the parameters on the next one. ::: dom/media/MediaFormatReader.cpp:1855 (Diff revision 6) > + [self, this, aTrack, > + &decoder](const MediaDataDecoder::DecodedData& aResults) { Weird formatting again. ::: dom/media/platforms/PlatformDecoderModule.h:235 (Diff revision 6) > virtual RefPtr<InitPromise> Init() = 0; > > - // Inserts a sample into the decoder's decode pipeline. > - virtual void Input(MediaRawData* aSample) = 0; > - > - // Causes all samples in the decoding pipeline to be discarded. When > + // Inserts a sample into the decoder's decode pipeline. The DecodePromise will > + // be resolved with the decoded MediaData. In case the decoder needs more > + // input, the DecodePromise may be resolved with an empty array of samples to > + // indicate the Decode should be called again before a MediaData is returned. 'the' -> 'that' ::: dom/media/platforms/PlatformDecoderModule.h:263 (Diff revision 6) > - // Called from the state machine task queue or main thread. > - // Decoder needs to decide whether or not hardware accelearation is supported > - // after creating. It doesn't need to call Init() before calling this function. > + // reader will delete the decoder once the promise is resolved. > + // The ShutdownPromise must only ever be resolved. > + virtual RefPtr<ShutdownPromise> Shutdown() = 0; > + > + // Called from the state machine task queue or main thread. Decoder needs to > + // decide whether or not hardware accelearation is supported after creating. 'accelearation' -> 'acceleration' ::: dom/media/platforms/agnostic/VorbisDecoder.cpp:249 (Diff revision 6) > + __func__); > } > > frames = vorbis_synthesis_pcmout(&mVorbisDsp, &pcm); > } > - > + return DecodePromise::CreateAndResolve(results, __func__); Move(results) ::: dom/media/platforms/agnostic/gmp/MediaDataDecoderProxy.cpp:61 (Diff revision 6) > + MOZ_ASSERT(!IsOnProxyThread()); > // Note that this *may* be called from the proxy thread also. This comment seems wrong, because of the new assertion above. ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:736 (Diff revision 6) > + RefPtr<DecodePromise> p; > + { > + MonitorAutoLock lock(mMonitor); > + p = mDecodePromise.Ensure(__func__); I don't think you need a {block} around the lock here, as it's the last thing in this function. This means you can regroup the declaration and initialization of 'p'. ::: dom/media/platforms/apple/AppleVTDecoder.cpp:454 (Diff revision 6) > // Frames come out in DTS order but we need to output them > // in composition order. > MonitorAutoLock mon(mMonitor); > mReorderQueue.Push(data); > if (mReorderQueue.Length() > mMaxRefFrames) { > - mCallback->Output(mReorderQueue.Pop().get()); > + mPromise.Resolve(DecodedData{Move(mReorderQueue.Pop())}, __func__); No need to Move() a function call as it's already an rvalue. ::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:359 (Diff revision 6) > { > RefPtr<MediaRawData> empty(new MediaRawData()); > empty->mTimecode = mLastInputDts; > bool gotFrame = false; > - while (NS_SUCCEEDED(DoDecode(empty, &gotFrame)) && gotFrame); > - mCallback->DrainComplete(); > + DecodedData results; > + while (NS_SUCCEEDED(DoDecode(empty, &gotFrame, results)) && gotFrame); Instead of the inconspicuous ';', could you please use an empty block '{}'? It can be on the same line. ::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:360 (Diff revision 6) > RefPtr<MediaRawData> empty(new MediaRawData()); > empty->mTimecode = mLastInputDts; > bool gotFrame = false; > - while (NS_SUCCEEDED(DoDecode(empty, &gotFrame)) && gotFrame); > - mCallback->DrainComplete(); > + DecodedData results; > + while (NS_SUCCEEDED(DoDecode(empty, &gotFrame, results)) && gotFrame); > + return DecodePromise::CreateAndResolve(results, __func__); Move(results) ::: dom/media/platforms/omx/OmxDataDecoder.cpp:392 (Diff revision 6) > > - if (self->mMediaRawDatas.Length()) { > + if (mMediaRawDatas.Length()) { > return; > } > > - LOGL("Call InputExhausted()"); > + mDecodePromise.ResolveIfExists(Move(mDecodedData), __func__); Are you using 'Move(mDecodedData)' and expecting that 'mDecodedData' will be empty afterward when you continue using it? (I've seen this pattern a few times already in this patch) This is dangerous, as the language does not guarantee that the moved-from object will be cleared! You are relying on the implementation of nsTArray to always do an actual move -- which it does (for now at least), lucky you! Maybe in the future there will be a short-array optimization, that would do a copy instead of a move in some cases. Unfortunately, there is no easy way to do this properly in a short-enough manner (shortest would be to add 'mDecodedData.Clear()' after this line), so I guess we'll just have to keep it for now; and it's probably done elsewhere anyway. But eventually someone will need to seriously look into this!
Attachment #8828952 - Flags: review?(gsquelart) → review+
Comment on attachment 8831094 [details] Bug 1319987: P9. More coding style fixes. https://reviewboard.mozilla.org/r/107746/#review109186 r+ with just a few nits... Assuming you've done some of these through a tool (clang-format?), it's not good enough that it was done automatically if it produces things that go against our coding style! So you'll have to update the rules in either the tool or the coding style, if you want 100% success. ;-) ::: dom/media/Benchmark.h:27 (Diff revision 1) > class Benchmark; > > class BenchmarkPlayback : public QueueObject > { > friend class Benchmark; > - explicit BenchmarkPlayback(Benchmark* aMainThreadState, MediaDataDemuxer* aDemuxer); > + BenchmarkPlayback(Benchmark* aMainThreadState, MediaDataDemuxer* aDemuxer); Note that 'explicit' can make sense with multi-parameter constructors. In this case, it would prevent implicit construction from an initializer list with two pointers, e.g. `{nullptr,nullptr}`. It's probably fine to remove the 'explicit' in this case, just don't feel obliged to. (Our coding style only says that one-parameter constructors *must* be 'explicit' or 'MOZ_IMPLICIT', it doesn't force anything about multi-parameter constructors.) ::: dom/media/Benchmark.h:64 (Diff revision 1) > struct Parameters > { > Parameters() > : mFramesToMeasure(-1) > , mStartupFrame(1) > - , mTimeout(TimeDuration::Forever()) {} > + , mTimeout(TimeDuration::Forever()) { } Move `{ }` over next two lines (as the whole constructor cannot fit in one line), just like you've doen with the next constructor. ::: dom/media/MediaDecoder.h:486 (Diff revision 1) > - virtual MediaDecoderOwner::NextFrameStatus NextFrameStatus() { return mNextFrameStatus; } > + virtual MediaDecoderOwner::NextFrameStatus NextFrameStatus() > + { > + return mNextFrameStatus; > + } > virtual MediaDecoderOwner::NextFrameStatus NextFrameBufferedStatus(); Would it be possible & reasonable & worth it to make these two functions 'const'? ::: dom/media/MediaDecoder.h:486 (Diff revision 1) > MOZ_ASSERT(NS_IsMainThread()); > MOZ_DIAGNOSTIC_ASSERT(!IsShutdown()); > GetOwner()->UpdateReadyState(); > } > > - virtual MediaDecoderOwner::NextFrameStatus NextFrameStatus() { return mNextFrameStatus; } > + virtual MediaDecoderOwner::NextFrameStatus NextFrameStatus() I can't see any overrides for NextFrameStatus(), does it need to be virtual? ::: dom/media/MediaDecoder.cpp:1045 (Diff revision 1) > - if (!IsNaN(mDuration) && !mozilla::IsInfinite<double>(mDuration) && length >= 0) { > + if (!IsNaN(mDuration) && !mozilla::IsInfinite<double>(mDuration) > + && length >= 0) { I don't like these half-split expressions. :-( ::: dom/media/MediaDecoder.cpp:1301 (Diff revision 1) > - mozilla::Abs(mEstimatedDuration.Ref().ref().ToMicroseconds() - aDuration) < ESTIMATED_DURATION_FUZZ_FACTOR_USECS) { > + && mozilla::Abs(mEstimatedDuration.Ref().ref().ToMicroseconds() > + - aDuration) < ESTIMATED_DURATION_FUZZ_FACTOR_USECS) { This split does not make sense. ::: dom/media/MediaDecoder.cpp:1346 (Diff revision 1) > - IsInfinite() ? > - media::TimeUnit::FromInfinity() : > - media::TimeUnit::FromSeconds(GetDuration()))); > + IsInfinite() > + ? media::TimeUnit::FromInfinity() > + : media::TimeUnit::FromSeconds(GetDuration()))); Left-align '?' and ':' with 'IsInfinite'. ::: dom/media/MediaDecoder.cpp:1463 (Diff revision 1) > - return mVideoFrameContainer ? mVideoFrameContainer->GetImageContainer() : nullptr; > + return mVideoFrameContainer > + ? mVideoFrameContainer->GetImageContainer() > + : nullptr; Alignment ::: dom/media/MediaDecoder.cpp:1631 (Diff revision 1) > > NS_IMETHODIMP > MediaMemoryTracker::CollectReports(nsIHandleReportCallback* aHandleReport, > nsISupports* aData, bool aAnonymize) > { > int64_t video = 0, audio = 0; I was going to complain about the two variables being RAII'd in the same line, but it seems our coding style doesn't care. Oh well. However, the coding style suggests that variables should be declared near their first use, which would be 32 lines below. ::: dom/media/MediaDecoder.cpp:1765 (Diff revision 1) > return GetBuffered().Contains(interval) > - ? MediaDecoderOwner::NEXT_FRAME_AVAILABLE > + ? MediaDecoderOwner::NEXT_FRAME_AVAILABLE > - : MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE; > + : MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE; Alignment ::: dom/media/MediaDecoderStateMachine.cpp:189 (Diff revision 1) > > class MediaDecoderStateMachine::StateObject > { > public: > - virtual ~StateObject() {} > - virtual void Exit() {}; // Exit action. > + virtual ~StateObject() { } > + virtual void Exit() { }; // Exit action. While you're here, please remove the unnecessary & inconsistent ';'. ::: dom/media/MediaDecoderStateMachine.cpp:2248 (Diff revision 1) > // soon anyway and we'll want to be able to display frames immediately > // after buffering finishes. We ignore the low audio calculations for > // readers that are async, as since their audio decode runs on a different > // task queue it should never run low and skipping won't help their decode. > - bool isLowOnDecodedAudio = !Reader()->IsAsync() && > - mMaster->IsAudioDecoding() && > + bool isLowOnDecodedAudio = > + !Reader()->IsAsync() && mMaster->IsAudioDecoding() Another unlikable split (first '&&' should be on new line). ::: dom/media/MediaDecoderStateMachine.cpp:2252 (Diff revision 1) > - LOW_VIDEO_THRESHOLD_USECS; > + (mMaster->GetClock() - mMaster->mDecodedVideoEndTime) > + * mMaster->mPlaybackRate > LOW_VIDEO_THRESHOLD_USECS; Weird split at wrong operator precedence. ::: dom/media/MediaDecoderStateMachine.cpp:2287 (Diff revision 1) > - shouldBuffer = IsExpectingMoreData() && > - mMaster->HasLowDecodedData() && > + shouldBuffer = IsExpectingMoreData() && mMaster->HasLowDecodedData() > + && mMaster->HasLowBufferedData(); :-( ::: dom/media/MediaDecoderStateMachine.cpp:2387 (Diff revision 1) > - elapsed < TimeDuration::FromSeconds(mBufferingWait * mMaster->mPlaybackRate) && > - mMaster->HasLowBufferedData(mBufferingWait * USECS_PER_S) && > + && elapsed > + < TimeDuration::FromSeconds(mBufferingWait * mMaster->mPlaybackRate) '<' should be aligned with 'elapsed' ::: dom/media/MediaDecoderStateMachine.cpp:2660 (Diff revision 1) > - ? new DecodedStream(mTaskQueue, mAbstractMainThread, mAudioQueue, mVideoQueue, > - mOutputStreamManager, mSameOriginMedia.Ref(), > - mMediaPrincipalHandle.Ref()) > + aAudioCaptured > + ? new DecodedStream(mTaskQueue, mAbstractMainThread, mAudioQueue, > + mVideoQueue, mOutputStreamManager, > + mSameOriginMedia.Ref(), mMediaPrincipalHandle.Ref()) > - : CreateAudioSink(); > + : CreateAudioSink(); ?: alignment ::: dom/media/MediaDecoderStateMachine.cpp:3201 (Diff revision 1) > - GetDecodedAudioDuration() < EXHAUSTED_DATA_MARGIN_USECS * mPlaybackRate; > + && GetDecodedAudioDuration() > + < EXHAUSTED_DATA_MARGIN_USECS * mPlaybackRate; '<' under 'Get...' ::: dom/media/MediaFormatReader.cpp:1171 (Diff revision 1) > } > > UniquePtr<TrackInfo> audioInfo = mAudio.mTrackDemuxer->GetInfo(); > // We actively ignore audio tracks that we know we can't play. > - audioActive = audioInfo && audioInfo->IsValid() && > - (!platform || > + audioActive = > + audioInfo && audioInfo->IsValid() First '&&' on new line please. ::: dom/media/MediaFormatReader.cpp:1313 (Diff revision 1) > return aSkipToNextKeyframe; > } > return (nextKeyframe < aTimeThreshold || > - (mVideo.mTimeThreshold && > - mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold)) && > - nextKeyframe.ToMicroseconds() >= 0 && !nextKeyframe.IsInfinite(); > + (mVideo.mTimeThreshold > + && mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold)) > + && nextKeyframe.ToMicroseconds() >= 0 && !nextKeyframe.IsInfinite(); Last '&&' on new line. ::: dom/media/MediaFormatReader.cpp:1424 (Diff revision 1) > - aSamples->mSamples[0]->mTrackInfo ? aSamples->mSamples[0]->mTrackInfo->GetID() : 0); > + aSamples->mSamples[0]->mTrackInfo > + ? aSamples->mSamples[0]->mTrackInfo->GetID() > + : 0); ?: alignment ::: dom/media/MediaFormatReader.cpp:1497 (Diff revision 1) > - aSamples->mSamples[0]->mTrackInfo ? aSamples->mSamples[0]->mTrackInfo->GetID() : 0); > + aSamples->mSamples[0]->mTrackInfo > + ? aSamples->mSamples[0]->mTrackInfo->GetID() > + : 0); ?: alignment ::: dom/media/MediaFormatReader.cpp:2473 (Diff revision 1) > } else { > mAudio.mSeekRequest.Complete(); > } > > if (aError == NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA) { > - if (HasVideo() && aTrack == TrackType::kAudioTrack && > + if (HasVideo() && aTrack == TrackType::kAudioTrack '&&' on new line. ::: dom/media/MediaFormatReader.cpp:2665 (Diff revision 1) > void > MediaFormatReader::NotifyDataArrived() > { > MOZ_ASSERT(OnTaskQueue()); > > - if (mShutdown || !mDemuxer || > + if (mShutdown || !mDemuxer '||' on new line. ::: dom/media/MediaInfo.h:282 (Diff revision 1) > // rectangle as it was reported relative to the picture size reported by the > // container. > nsIntRect ScaledImageRect(int64_t aWidth, int64_t aHeight) const > { > - if ((aWidth == mImage.width && aHeight == mImage.height) || > - !mImage.width || !mImage.height) { > + if ((aWidth == mImage.width && aHeight == mImage.height) > + || !mImage.width || !mImage.height) { Last '||' on new line. ::: dom/media/MediaInfo.h:511 (Diff revision 1) > { > NS_ASSERTION(!HasAudio() || mAudio.mTrackId != TRACK_INVALID, > "Audio track ID must be valid"); > NS_ASSERTION(!HasVideo() || mVideo.mTrackId != TRACK_INVALID, > "Audio track ID must be valid"); > - NS_ASSERTION(!HasAudio() || !HasVideo() || > + NS_ASSERTION(!HasAudio() || !HasVideo() '||' on new line. ::: dom/media/MediaInfo.h:584 (Diff revision 1) > { > return mInfo ? mInfo->GetAsTextInfo() : nullptr; > } > > private: > - ~SharedTrackInfo() {}; > + ~SharedTrackInfo() { }; Remove unnecessary and inconsistent ';' ::: dom/media/MediaInfo.h:659 (Diff revision 1) > - bool IsValid() const { > + bool IsValid() const > + { > return mValid; > } This could probably completely fit in one line :-) ::: dom/media/MediaInfo.h:727 (Diff revision 1) > return mInterleaved; > } > bool operator==(const AudioConfig& aOther) const > { > - return mChannelLayout == aOther.mChannelLayout && > - mRate == aOther.mRate && mFormat == aOther.mFormat && > + return mChannelLayout == aOther.mChannelLayout > + && mRate == aOther.mRate && mFormat == aOther.mFormat Second '&&' on new line.
Attachment #8831094 - Flags: review?(gsquelart) → review+
Flags: needinfo?(gsquelart)
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109158 > Isn't this a DecodePromise, where the resolve function takes a list of decoded samples? > > Does MozPromise let you define lambdas that don't actually take the parameters needed? > > More importantly, don't we need to actually take those samples and send them back to the child? > > I think we want to change PVideoDecoder.ipdl to more closely match the new MediaDataDecoder definition. > > We can replace 'Output' and 'InputExhausted' with 'ResolveDecodePromise' and 'RejectDecodePromise' (with arguments that match the real promise) and call these from both Init and Drain. yes you're right of course... brain fart here... will fix
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109158 > yes you're right of course... brain fart here... > will fix About the rest of the definition of IPDL. I'm quite found of the Output / InputExhausted API, it makes it quite easy to integrate with existing out of process code (for both the Android's RemoteDataDecoder and VideoDecoderChild/ Changing the IPDL ot better match the promise methods was something I was quite wary about, as I'm not super familiar with those. But I can look into those of course.
Comment on attachment 8831606 [details] Bug 1319987: P10. Update WPT expected results. https://reviewboard.mozilla.org/r/108158/#review109206
Attachment #8831606 - Flags: review?(gsquelart) → review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109160 > While you're fixing the style, could you please change the C-style cast into C++? that's done in P9 > It'd be great to make this promise-based too, instead of blocking! (Probably for another bug...) to not mess up with the benchmark data, I think we do want to not have much else running at the same time.. so being a blocker here isn't too bed. But in any case, under most cases, the taskqueue should be empty by then and it should be a no-op > Style: If you start breaking lines in repeated-operator expression, I think each operand should be on a separate line. I.e.: > ``` > && (A > || B > || C) > ``` > Otherwise at a glance it looks like there are only two arguments. > > (If that's automated, fix the formatter, or fix its shoddy work before saving!) yes it is magic clang that does that. But there's a fair amount of manual editting because I don't find clang changes always that good. It makes maximum use of the 80 columns, so if it fits in 80 columns it will make use of it. I don't see anything in the coding style however that an expression must be placed on a new line. so: A || B || C it's perfectly fine > That's quite a bit of non-generic and non-critical code in a header, could you move it to the cpp file? that would be a change in the code structure. For now there is no code in the cpp related to DecoderData > Style: Align '?' and ':' with 'mType'. that's not how clang-format with mozilla style does it, And personally I agree with clang-format there :) > I noticed that you do these two DisconnectIfExists() in ShutdownDecoder() above, so do you still need them here? probably not > No need to set mShutdown to true before this return? (as you've done a bit below) no... mShutdown is a member of the base class. MediaDecoderReader::Shutdown sets mShutdown to true already. I only set mShutdown to true earlier so that we can cancel any other actions before Shutdown has completed. > This comment seems wrong, because of the new assertion above. I added the assert because the comment seems wrong to me too... > No need to Move() a function call as it's already an rvalue. can't hurt :) > Are you using 'Move(mDecodedData)' and expecting that 'mDecodedData' will be empty afterward when you continue using it? > (I've seen this pattern a few times already in this patch) > > This is dangerous, as the language does not guarantee that the moved-from object will be cleared! > You are relying on the implementation of nsTArray to always do an actual move -- which it does (for now at least), lucky you! > Maybe in the future there will be a short-array optimization, that would do a copy instead of a move in some cases. > > Unfortunately, there is no easy way to do this properly in a short-enough manner (shortest would be to add 'mDecodedData.Clear()' after this line), so I guess we'll just have to keep it for now; and it's probably done elsewhere anyway. > But eventually someone will need to seriously look into this! Yes I do (do it on purpose) The constructor of nsTArray<T>(T&&) clearly does what needs to be done. It calls nsTarray::SwapElements() Same for operator=(self_type&& aOther): if (this != &aOther) { Clear(); SwapElements(aOther); } So this will move the *elements* of the array, while leaving the array cleared but still usable.
Comment on attachment 8831094 [details] Bug 1319987: P9. More coding style fixes. https://reviewboard.mozilla.org/r/107746/#review109186 > Would it be possible & reasonable & worth it to make these two functions 'const'? maybe.. but that's out of scope > I can't see any overrides for NextFrameStatus(), does it need to be virtual? also out of scope for this bug > This split does not make sense. sure, but how else who you split it?
Blocks: 1335179
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109436 Looks good! Few small comments, but r=me with those fixed. Looking forward to bug 1335179 :) ::: dom/media/ipc/VideoDecoderChild.cpp:64 (Diff revision 8) > > mozilla::ipc::IPCResult > VideoDecoderChild::RecvInputExhausted() > { > AssertOnManagerThread(); > - if (mCallback) { > + mDecodePromise.ResolveIfExists(Move(mDecodedFrames), __func__); Can we just make this Resolve, not ResolveIfExists? It shouldn't be getting called unless we called Decode. Same with DrainComplete and FlushComplete. ::: dom/media/ipc/VideoDecoderChild.cpp:126 (Diff revision 8) > GetManager()->RunWhenRecreated(NS_NewRunnableFunction([=]() { > - if (ref->mInitialized && ref->mCallback) { > - ref->mCallback->Error(NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER); > + if (ref->mInitialized) { > + mDecodedFrames.Clear(); > + mDecodePromise.Reject(NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER, __func__); > + mDrainPromise.Reject(NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER, __func__); > + mFlushPromise.Reject(NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER, __func__); These three should all be RejectIfExists right? ::: dom/media/ipc/VideoDecoderChild.cpp:204 (Diff revision 8) > + return MediaDataDecoder::DecodePromise::CreateAndReject( > + NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER, __func__); > + } > if (!mCanSend) { > - return; > + return MediaDataDecoder::DecodePromise::CreateAndReject( > + NS_ERROR_DOM_MEDIA_DECODE_ERR, __func__); We don't want to return a DECODE_ERR here, as that is fatal and will end playback of the video. If mCanSend if false then the IPC channel has died, but if !mNeedNewDecoder then we're still waiting for the RunWhenRecreated lambda to execute (line 122). We should just call mDecoderPromise.Ensure() here, and then when the RunWhenRecreated code executes it'll reject the promise for us (with NEED_NEW_DECODER). ::: dom/media/ipc/VideoDecoderParent.cpp:202 (Diff revision 8) > + Unused << SendFlushComplete(); > - } > + } > - })); > + }, > + [self, this]() { > + if (!mDestroyed) { > + Unused << SendFlushComplete(); Should we be sending Error instead of FlushComplete here? Please add a comment as to why we ignore the failed promise here if not.
Attachment #8828952 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109160 > yes it is magic clang that does that. But there's a fair amount of manual editting because I don't find clang changes always that good. It makes maximum use of the 80 columns, so if it fits in 80 columns it will make use of it. > I don't see anything in the coding style however that an expression must be placed on a new line. > so: > A > || B || C > it's perfectly fine It's true that the coding style doesn't mention that, but it seems the "right thing" to do, to make our code as readable as possible, and I just worry that expressions like: A || B || C can make 'C' hard to spot in some situations. > that's not how clang-format with mozilla style does it, And personally I agree with clang-format there :) Hey, if you don't like it, get the style changed! https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators > I added the assert because the comment seems wrong to me too... So I would suggest you just remove this comment, so as not to confuse future generations. > Yes I do (do it on purpose) > > The constructor of nsTArray<T>(T&&) clearly does what needs to be done. > It calls nsTarray::SwapElements() > > Same for operator=(self_type&& aOther): > if (this != &aOther) { > Clear(); > SwapElements(aOther); > } > > So this will move the *elements* of the array, while leaving the array cleared but still usable. My point was that you are relying on the current implementation of nsTArray's move-constructor that empties the moved-from array, but it is not guaranteed by the language, and it would be perfectly valid for some implementation to choose not to empty the original array if that can be more optimal in some cases (e.g., array with some internal storage, like AutoTArray). Also, `Move()` itself doesn't do anything, it's just a hint, and you are relying on ResolveIfExists' implementation to use move-semantics. At the minimum I would recommend that you add a `MOZ_ASSERT(mDecodedData.IsEmpty());` after the move, to make it clear that you want the array to be emptied, and to catch future issues if one of the many assumptions breaks one day. (Re-opening the issue, so it's more visible, as I think it's important; feel free to close it again and I'll go away)
Comment on attachment 8831094 [details] Bug 1319987: P9. More coding style fixes. https://reviewboard.mozilla.org/r/107746/#review109186 > sure, but how else who you split it? Splits should happen at the lowest precedence first, so in this case: ``` if (mEstimatedDuration.Ref().isSome() && mozilla::Abs(mEstimatedDuration.Ref().ref().ToMicroseconds() - aDuration) < ESTIMATED_DURATION_FUZZ_FACTOR_USECS) { ```
Attachment #8831092 - Flags: review?(cpearce) → review?(gsquelart)
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109160 > My point was that you are relying on the current implementation of nsTArray's move-constructor that empties the moved-from array, but it is not guaranteed by the language, and it would be perfectly valid for some implementation to choose not to empty the original array if that can be more optimal in some cases (e.g., array with some internal storage, like AutoTArray). > > Also, `Move()` itself doesn't do anything, it's just a hint, and you are relying on ResolveIfExists' implementation to use move-semantics. > > At the minimum I would recommend that you add a `MOZ_ASSERT(mDecodedData.IsEmpty());` after the move, to make it clear that you want the array to be emptied, and to catch future issues if one of the many assumptions breaks one day. > > (Re-opening the issue, so it's more visible, as I think it's important; feel free to close it again and I'll go away) ok, I'll add MOZ_ASSERT(mDecodedData.IsEmpty()); everywhere it's used
Comment on attachment 8831092 [details] Bug 1319987: P7. Re-implement handling for WaitingForKey in MFR. https://reviewboard.mozilla.org/r/107742/#review109554 r+ with question: ::: dom/media/MediaFormatReader.cpp:1538 (Diff revision 3) > + if (!decoder.mDecodeRequest.Exists()) { > + LOGV("WaitingForKey received while no pending decode. Ignoring"); > + } I'm a bit confused: The log message says "Ignoring", but the executed code is exactly the same! Are you missing a 'return;' in the if-block? (If not, just remove "Ignoring")
Attachment #8831092 - Flags: review?(gsquelart) → review+
Comment on attachment 8831092 [details] Bug 1319987: P7. Re-implement handling for WaitingForKey in MFR. https://reviewboard.mozilla.org/r/107742/#review109554 > I'm a bit confused: The log message says "Ignoring", but the executed code is exactly the same! Are you missing a 'return;' in the if-block? (If not, just remove "Ignoring") it's just for logging... as no mDecodeRequest exists, when Update() gets to run (line 2121) it will do: if (decoder.mWaitingForKey) { decoder.mWaitingForKey = false; ... which pretty much cancel the thing. Reason that code is there is it's what was there before P1 when the code was removed. I guess it could be useful for logging. or I could remove it altogether.
Attachment #8831092 - Flags: review?(cpearce)
Comment on attachment 8832215 [details] Bug 1319987: P11. Fix MediaDataDecoder gtest. https://reviewboard.mozilla.org/r/108560/#review109726
Attachment #8832215 - Flags: review?(gsquelart) → review+
Comment on attachment 8831092 [details] Bug 1319987: P7. Re-implement handling for WaitingForKey in MFR. https://reviewboard.mozilla.org/r/107742/#review109732
Attachment #8831092 - Flags: review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review108804 ::: dom/media/platforms/agnostic/eme/SamplesWaitingForKey.h:33 (Diff revision 5) > - explicit SamplesWaitingForKey(MediaDataDecoder* aDecoder, > - MediaDataDecoderCallback* aCallback, > - TaskQueue* aTaskQueue, > - CDMProxy* aProxy); > + typedef MozPromise<RefPtr<MediaRawData>, bool, /* IsExclusive = */ true> > + WaitForKeyPromise; > + > + explicit SamplesWaitingForKey(CDMProxy* aProxy); > > // Returns true if we need to wait for a key to become usable. Comment is out of date. ::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:123 (Diff revision 9) > > void > -VideoCallbackAdapter::Error(GMPErr aErr) > +GMPVideoDecoder::Error(GMPErr aErr) > { > MOZ_ASSERT(IsOnGMPThread()); > - mCallback->Error(MediaResult(aErr == GMPDecodeErr > + RefPtr<GMPVideoDecoder> self = this; Why are you holding a self reference here? I don't see you using the self reference anywhere. And if there's a good reason for doing it here, why don't you do it in GMPVideoDecoder::Terminated()?
Attachment #8828952 - Flags: review?(cpearce) → review+
Comment on attachment 8828952 [details] Bug 1319987: P5. Promisify MediaDataDecoder. https://reviewboard.mozilla.org/r/106180/#review109436 > Can we just make this Resolve, not ResolveIfExists? > > It shouldn't be getting called unless we called Decode. > > Same with DrainComplete and FlushComplete. Flush may have been called in between; so we are in effect ignoring the output. the decode and drain promise should have been rejected in Flush() > These three should all be RejectIfExists right? of course ! > We don't want to return a DECODE_ERR here, as that is fatal and will end playback of the video. > > If mCanSend if false then the IPC channel has died, but if !mNeedNewDecoder then we're still waiting for the RunWhenRecreated lambda to execute (line 122). > > We should just call mDecoderPromise.Ensure() here, and then when the RunWhenRecreated code executes it'll reject the promise for us (with NEED_NEW_DECODER). DECODE_ERR aren't fatal; it will make the MFR retry, at which case the promise will be immediately rejected with NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER. but I can make create the promise under all those calls too that check mCanSend
Blocks: 1336358
There are now two issues blocking this bug, all android related. First one is the RemoteDataDecoder that returns event in the incorrect order or doesn't always return the sample with the flag BUFFER_FLAG_END_OF_STREAM (bug 1336358). All frames are returned, just not the empty frame with the flag BUFFER_FLAG_END_OF_STREAM. Looking at the android spec, I see no mention that this frame should be returned, just that it will perform a drain (https://developer.android.com/reference/android/media/MediaCodec.html#BUFFER_FLAG_END_OF_STREAM and that's not available on all API). So maybe we should look at the DurationQueue size instead, and if empty returns DrainComplete. The second bug is related to the SupportDecoderRecycling. The code is racy on accessing the VideoInfo and is causing bug 1336431
Flags: needinfo?(snorp)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jolin)
Flags: needinfo?(jacheng)
Flags: needinfo?(bwu)
Flags: needinfo?(ayang)
Comment on attachment 8833233 [details] Bug 1319987: P12. Disable RemoteDataDecoder. ?jhlin https://reviewboard.mozilla.org/r/109468/#review110922
Attachment #8833233 - Flags: review+
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20e6f90089fa P1. Remove handling for WaitingForKey in MFR. r=cpearce https://hg.mozilla.org/integration/autoland/rev/fd747ba4db08 P2. Remove FuzzingWrapper. r=gerald https://hg.mozilla.org/integration/autoland/rev/1a05ce09ead4 P3. Remove Gonk PDM. r=cpearce https://hg.mozilla.org/integration/autoland/rev/66f931421288 P4. Refactor H264 Converter. r=cpearce https://hg.mozilla.org/integration/autoland/rev/b2171e3e8b69 P5. Promisify MediaDataDecoder. r=cpearce,gerald,mattwoodrow,snorp https://hg.mozilla.org/integration/autoland/rev/c523a15a402f P6. Shutdown demuxer asynchronously. r=gerald https://hg.mozilla.org/integration/autoland/rev/3f1edb00f343 P7. Re-implement handling for WaitingForKey in MFR. r=cpearce,gerald https://hg.mozilla.org/integration/autoland/rev/b45df222b0ee P8. Fix comment. r=gerald https://hg.mozilla.org/integration/autoland/rev/7ab9b3dd619c P9. More coding style fixes. r=gerald https://hg.mozilla.org/integration/autoland/rev/d42edade7ed8 P10. Update WPT expected results. r=gerald https://hg.mozilla.org/integration/autoland/rev/8f062bf0099d P11. Fix MediaDataDecoder gtest. r=gerald https://hg.mozilla.org/integration/autoland/rev/ff9f57defc04 P12. Disable RemoteDataDecoder. ?jhlin r=jya
(In reply to Jean-Yves Avenard [:jya] from comment #113) > There are now two issues blocking this bug, all android related. > > First one is the RemoteDataDecoder that returns event in the incorrect order > or doesn't always return the sample with the flag BUFFER_FLAG_END_OF_STREAM > (bug 1336358). All frames are returned, just not the empty frame with the > flag BUFFER_FLAG_END_OF_STREAM. Looking at the android spec, I see no > mention that this frame should be returned, just that it will perform a > drain > (https://developer.android.com/reference/android/media/MediaCodec. > html#BUFFER_FLAG_END_OF_STREAM and that's not available on all API). So > maybe we should look at the DurationQueue size instead, and if empty returns > DrainComplete. Pretty sure MediaCodec will return BUFFER_FLAG_END_OF_STREAM frame if it received input buffer with that flag on. Will apply your patch and see what goes wrong.
Flags: needinfo?(jolin)
I just saw you store the VideoInfo by copy instead of reference in your P5. This is the fix for the racy right?
Flags: needinfo?(jacheng)
(In reply to James Cheng[:JamesCheng] from comment #137) > I just saw you store the VideoInfo by copy instead of reference in your P5. > This is the fix for the racy right? Hardly a fix :) and for now I've disabled the RemoteDataDecoder as it returns events incorrectly and is incompatible with promises
Depends on: 1337265
Flags: needinfo?(bwu)
Depends on: 1339449
See Also: → 1340580
Depends on: 1341210
Depends on: 1345342
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: