Currently we create a MediaDataDecoder, then call Init() on it. After that there's a whole work around on platform where it may take time for the MediaDataDecoder to initialize (gonk) so that it won't attempt to use the decoder until it's actually ready. This should all be simplified by having MediaDataDecoder::Init() return a MediaPromise that will resolve only once the MediaDataDecoder has been initialized and is ready to use. This will help with bug 1132832, and allow for a more elegant design than what's currently in place

This will go very nicely with bug 1136873. Combined with this, there will be no more need for checking repeatedly if a decoder is usable or not.

Having MediaDataDecoder Init return a promise sounds like a great idea.

H264 decoder should be able to accept new SPS/PPS without restarting. For example in Gonk, the H264 OpenMax decoder sends a 'FormatChangeEvent' event when decoding a new SPS/PPS. Other H264 platform decoder should have the similar behaviour. Do we really need to handle SPS/PPS in this H264Converter?

Comment on attachment 8626504 [details] [diff] [review] pdm_init_by_promise_platform Review of attachment 8626504 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes addressed. This is just an intermediary step right? The main goal of using media promises for initializing the decoder is for the gonk platform. So that the gonk decoder won't wait until the decoder is ready and get rid of this terrible WAIT_FOR_RESOURCES mechanism where we test/retry until the initialisation succeeds. The idea is that the MediaDataDecoder returns a promise and resolve it once its done. No more WAIT_FOR_RESOURCES being returned by ReadMetadata / AsyncReadMetadata. Where is that change ? that's the most interesting one. If you intend to implement this in another bug, then please link to it here and make this bug blocks it. ::: dom/media/platforms/PlatformDecoderModule.h @@ +206,5 @@ > protected: > virtual ~MediaDataDecoder() {}; > > public: > + enum NotInitedReason { DecoderFailureReason While it's currently only used for init, it could be used later for other types. @@ +207,5 @@ > virtual ~MediaDataDecoder() {}; > > public: > + enum NotInitedReason { > + INITED_ERROR, INIT_ERROR inited isn't a word ... @@ +208,5 @@ > > public: > + enum NotInitedReason { > + INITED_ERROR, > + HARDWARE_NOT_AVAILABLE, You seem to report HARDWARE_NOT_AVAILABLE for some init errors. This is confusing. Hardware may not always be used. As currently if hardware isn't available then a software decoder will be used (like the Apple PDM or the WMF one); when would you ever need to report HARDWARE_NOT_AVAILABLE? @@ +219,4 @@ > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDataDecoder) > > // Initialize the decoder. The decoder should be ready to decode after > + // promise returns. The decoder should do any initialization here, rather once the promise resolves. ::: dom/media/platforms/SharedDecoderManager.cpp @@ +72,5 @@ > SharedDecoderManager::SharedDecoderManager() > : mTaskQueue(new FlushableMediaTaskQueue(GetMediaThreadPool(MediaThreadType::PLATFORM_DECODER))) > , mActiveProxy(nullptr) > , mActiveCallback(nullptr) > + , mInited(false) mInitDone , or mInit @@ +93,5 @@ > layers::LayersBackend aLayersBackend, > layers::ImageContainer* aImageContainer, > FlushableMediaTaskQueue* aVideoTaskQueue, > + MediaDataDecoderCallback* aCallback, > + MediaDecoderReader* aReader) why do you want to pass the reader ? you only use it to access its task queue, you should be using the SharedDecoderManager's task queue instead. @@ +141,5 @@ > mImageContainer); > if (!mDecoder) { > return false; > } > + return true; if you recreate the decoder, shouldn't you mark that it hasn't been initialised yet ? e.g. set mInit to false? @@ +186,5 @@ > + nsRefPtr<SharedDecoderManager> self = this; > + nsRefPtr<MediaDataDecoder::InitPromise> p = mDecoderInitPromise.Ensure(__func__); > + > + mDecoder->Init() > + ->Then(mReader->TaskQueue(), __func__, why don't you use the SDM task queue to initialise the decoder? ::: dom/media/platforms/agnostic/gmp/MediaDataDecoderProxy.h @@ +43,5 @@ > return NS_OK; > } > > + nsRefPtr<MediaDataDecoder::InitPromise> GetInitPromise() { > + return mPromise; this is very cumbersome. If you need to pass the promise resolved by another, we should be using ProxyMediaCall instead rather than creating a task that would later export its promise. I guess this can be done in a follow-up task, but it's a bit of an ugly setup when using promises and certainly not using all their goodness ::: dom/media/platforms/apple/AppleVDADecoder.cpp @@ +517,5 @@ > { > nsRefPtr<AppleVDADecoder> decoder = > new AppleVDADecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > + > + // XXX it needs to find another way to ensure the decoder supporting hardware to ensure the decoder supports hardware acceleration. Can you please open a follow up bug for this ? though I can't see how it could be done any other way ::: dom/media/platforms/apple/AppleVTDecoder.cpp @@ +60,5 @@ > + if (NS_SUCCEEDED(rv)) { > + return InitPromise::CreateAndResolve(TrackType::kVideoTrack, __func__); > + } > + > + return InitPromise::CreateAndReject(NotInitedReason::HARDWARE_NOT_AVAILABLE, __func__); this is an invalid error result. VT can be software ; just return INIT_ERROR

Comment on attachment 8626505 [details] [diff] [review] pdm_init_by_promise Review of attachment 8626505 [details] [diff] [review]: ----------------------------------------------------------------- You need to rebase this patch ; in particular to handle re-creating the decoder following bug 1171314. You appear to have been missing that change ; and this is going to be the most difficult thing to handle. Otherwise looks good so far. ::: dom/media/MediaFormatReader.cpp @@ +282,4 @@ > mDemuxerInitRequest.Begin(mDemuxer->Init() > + ->Then(TaskQueue(), __func__, this, > + &MediaFormatReader::OnDemuxerInitDone, > + &MediaFormatReader::OnDemuxerInitFailed)); why change this formatting here? only adds noise to the patch @@ +445,5 @@ > mLayersBackendType, > mDecoder->GetImageContainer(), > mVideo.mTaskQueue, > + mVideo.mCallback, > + this); as mentioned in the other patch, you don't need to pass that last argument. @@ +462,5 @@ > } > > +uint32_t > +MediaFormatReader::DoDecodersInit() > +{ MOZ_ASSERT(OnTaskQueue()) @@ +477,5 @@ > + if (promises.Length()) { > + MediaDataDecoder::InitPromise::All(TaskQueue(), promises) > + ->Then(TaskQueue(), __func__, this, > + &MediaFormatReader::OnDecoderInitDone, > + &MediaFormatReader::OnDecoderInitFailed); return false @@ +483,5 @@ > + > + LOG("init decoders: audio: %p, audio init: %d, video: %p, video init: %d", > + mAudio.mDecoder.get(), mAudio.mDecoderInited, mVideo.mDecoder.get(), mVideo.mDecoderInited); > + > + return promises.Length(); return true @@ +488,5 @@ > +} > + > +void > +MediaFormatReader::OnDecoderInitDone(const nsTArray<TrackType>& aTrackTypes) > +{ MOZ_ASSERT(OnTaskQueue()) @@ +503,5 @@ > + return; > + } > + > + if (!mVideo.mPromise.IsEmpty()) { > + ScheduleUpdate(TrackType::kVideoTrack); You will want to schedule an update if aTrackTypes contains TrackType::kVideoTrack and not just if there's a pending promise as it could otherwise break the pending changing for decode-ahead @@ +507,5 @@ > + ScheduleUpdate(TrackType::kVideoTrack); > + return; > + } > + > + if (!mAudio.mPromise.IsEmpty()) { same here. @@ +515,5 @@ > +} > + > +void > +MediaFormatReader::OnDecoderInitFailed(MediaDataDecoder::NotInitedReason aReason) > +{ MOZ_ASSERT(OnTaskQueue()); @@ +524,5 @@ > + return; > + } > + > + if (!mVideo.mPromise.IsEmpty()) { > + mVideo.mPromise.Reject(CANCELED, __func__); why CANCELED? we failed to initialize the decoder ... it should be returning an error. I don't really like resolving the mediadata promise here ; it should be resolved in Update() ; call NotifyError(TrackType::kVideo) @@ +525,5 @@ > + } > + > + if (!mVideo.mPromise.IsEmpty()) { > + mVideo.mPromise.Reject(CANCELED, __func__); > + return; don't return. you may also need to notify that audio has failed @@ +529,5 @@ > + return; > + } > + > + if (!mAudio.mPromise.IsEmpty()) { > + mAudio.mPromise.Reject(CANCELED, __func__); same here: NotifyError(TrackType::kAudio); @@ +621,5 @@ > return p; > } > > nsRefPtr<VideoDataPromise> p = mVideo.mPromise.Ensure(__func__); > + if(!DoDecodersInit()) { If (DecodersInit() and add a comment that an update will be scheduled once the initialization promise is resolved. @@ +709,5 @@ > return AudioDataPromise::CreateAndReject(DECODE_ERROR, __func__); > } > > nsRefPtr<AudioDataPromise> p = mAudio.mPromise.Ensure(__func__); > + if (!DoDecodersInit()) { same as above ::: dom/media/MediaFormatReader.h @@ +112,5 @@ > void NotifyDemuxer(uint32_t aLength, int64_t aOffset); > void ReturnOutput(MediaData* aData, TrackType aTrack); > > + bool EnsureDecodersCreated(); > + uint32_t DoDecodersInit(); EnsureDecodersInitialized() to be consistent. And you only ever test if the returned value is 0; returns a bool then. And document what the returned value indicates: if false there's a pending initialization If true all decoders are initialized. I think EnsureDecodersCreated and EnsureDecodersInitialized should be merged. it's starting to get messy. @@ +198,5 @@ > , mDemuxEOSServiced(false) > , mWaitingForData(false) > , mReceivedNewData(false) > , mDiscontinuity(true) > + , mDecoderInited(false) mDecoderInitialized @@ +230,5 @@ > bool mDemuxEOSServiced; > bool mWaitingForData; > bool mReceivedNewData; > bool mDiscontinuity; > + bool mDecoderInited; mDecoderInitialized

(In reply to Alfredo Yang from comment #5) > Created attachment 8626513 [details] [diff] [review] > pdm_init_by_promise_h264converter > > H264 decoder should be able to accept new SPS/PPS without restarting. For > example in Gonk, the H264 OpenMax decoder sends a 'FormatChangeEvent' event > when decoding a new SPS/PPS. Other H264 platform decoder should have the > similar behaviour. > Do we really need to handle SPS/PPS in this H264Converter? not all decoders can accept new sps without restarting. only those supporting annexB (WMF and gonk) also decoders that do accept annexB will not be re-created and instead will be reused.. we also need parsing the SPS to properly detect chane of aspect ratio as not all platforms support this. We also decided to shutdown the decoder and recreate a new one in bug 1171314 so we have one code path across all platforms.

Comment on attachment 8626513 [details] [diff] [review] pdm_init_by_promise_h264converter Review of attachment 8626513 [details] [diff] [review]: ----------------------------------------------------------------- This breaks ConfigurationChanged(mCurrentConfig); MediaDataDecoder API

(In reply to Jean-Yves Avenard [:jya] from comment #6) > Comment on attachment 8626504 [details] [diff] [review] > pdm_init_by_promise_platform > > Review of attachment 8626504 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with changes addressed. > > This is just an intermediary step right? > > The main goal of using media promises for initializing the decoder is for > the gonk platform. So that the gonk decoder won't wait until the decoder is > ready and get rid of this terrible WAIT_FOR_RESOURCES mechanism where we > test/retry until the initialisation succeeds. > The idea is that the MediaDataDecoder returns a promise and resolve it once > its done. No more WAIT_FOR_RESOURCES being returned by ReadMetadata / > AsyncReadMetadata. > > Where is that change ? that's the most interesting one. > > If you intend to implement this in another bug, then please link to it here > and make this bug blocks it. Yes, I plan to solve the WAIT_FOR_RESOURCE on gonk platform after this bug. Bug 1179667. > @@ +186,5 @@ > > + nsRefPtr<SharedDecoderManager> self = this; > > + nsRefPtr<MediaDataDecoder::InitPromise> p = mDecoderInitPromise.Ensure(__func__); > > + > > + mDecoder->Init() > > + ->Then(mReader->TaskQueue(), __func__, > > why don't you use the SDM task queue to initialise the decoder? The task queue in SDM is a FlushableMediaTaskQueue which unfortunately can't be used in MediaPromise. That's the reason I use reader task queue here. Actually, I don't like to use reader task queue in SDM either. But I don't know how to get around MediaPromise restriction. Any suggestion? > > ::: dom/media/platforms/agnostic/gmp/MediaDataDecoderProxy.h > @@ +43,5 @@ > > return NS_OK; > > } > > > > + nsRefPtr<MediaDataDecoder::InitPromise> GetInitPromise() { > > + return mPromise; > > this is very cumbersome. > > If you need to pass the promise resolved by another, we should be using > ProxyMediaCall instead rather than creating a task that would later export > its promise. > > I guess this can be done in a follow-up task, but it's a bit of an ugly > setup when using promises and certainly not using all their goodness I'll write a nsIThread task queue wrapper to use ProxyMediaCall here. > > ::: dom/media/platforms/apple/AppleVDADecoder.cpp > @@ +517,5 @@ > > { > > nsRefPtr<AppleVDADecoder> decoder = > > new AppleVDADecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > > + > > + // XXX it needs to find another way to ensure the decoder supporting hardware > > to ensure the decoder supports hardware acceleration. > > Can you please open a follow up bug for this ? > though I can't see how it could be done any other way Bug 1179681.

Carry r+

Comment on attachment 8631508 [details] [diff] [review] pdm_init_by_promise Review of attachment 8631508 [details] [diff] [review]: ----------------------------------------------------------------- We starting to have too many codepath, and it's only muddying the picture. I'd much prefer if you handled the creation and initialization of the decoders within DecodeDemuxedSamples(). So after if (info && decoder.mLastStreamSourceID != info->GetID()) { ... } no we have: else { ... if (!EnsureDecodersCreated()) { NotifyErrors(aTrack); return; } if (EnsureDecodersInitialized()) { .... Also, I don't like this use of UpdateIfStreamSourceIDChange you don't need it. If DecodeDemuxedSamples() detects a change of stream, it sets the new stream ID; so the next call to DecodeDemuxedSamples() will complete just fine. Simply reschedule an update once the initialization has complete. Would make your patch much smaller too. ::: dom/media/MediaFormatReader.cpp @@ +376,5 @@ > metadata->mInfo = mInfo; > metadata->mTags = nullptr; > mMetadataPromise.Resolve(metadata, __func__); > + return; > + } else { you don't need else if you return earlier. @@ +446,5 @@ > mInfo.mAudio, > mAudio.mTaskQueue, > mAudio.mCallback); > NS_ENSURE_TRUE(mAudio.mDecoder != nullptr, false); > + mAudio.mDecoderInitialized = false; move this above line 443, so it's always set to false, including when a failure occurs @@ +475,5 @@ > mLayersBackendType, > mDecoder->GetImageContainer()); > } > NS_ENSURE_TRUE(mVideo.mDecoder != nullptr, false); > + mVideo.mDecoderInitialized = false; same here @@ +507,5 @@ > + &MediaFormatReader::OnDecoderInitFailed); > + } > + > + LOG("init decoders: audio: %p, audio init: %d, video: %p, video init: %d", > + mAudio.mDecoder.get(), mAudio.mDecoderInitialized, mVideo.mDecoder.get(), mVideo.mDecoderInitialized); 80 columns formatting @@ +523,5 @@ > + auto& decoder = GetDecoderData(aTrackTypes[i]); > + decoder.mDecoderInitialized = true; > + > + // Decoder is reinitialized due to stream source id changed; > + if (UpdateIfStreamSourceIDChange(aTrackTypes[i])) { you don't need this. This will always return false as the mLastStreamSourceID has already been updated. DecodeDemuxedSamples will take care of completing the task. @@ +528,5 @@ > + return; > + } > + > + if (mMetadataPromise.IsEmpty()) { > + ScheduleUpdate(aTrackTypes[i]); Always call ScheduleUpdate(), let Update() sort it out. You should set mInitDone once you have completed the initialisation of the decoder only. Currently this is done once the demuxer has finished to initialize, because after that, the decoder's initialization was synchronous. That assumption is now false. You should set mInitDone when you finish this loop, so we won't end up with a reader that has mInitDone set to true, yet is unusable @@ +548,5 @@ > + MOZ_ASSERT(OnTaskQueue()); > + LOG("failed to init decoder"); > + > + if (!mMetadataPromise.IsEmpty()) { > + mMetadataPromise.Reject(ReadMetadataFailureReason::METADATA_ERROR, __func__); RejectIfExists, no need for the test @@ +553,5 @@ > + return; > + } > + > + if (!mVideo.mPromise.IsEmpty()) { > + NotifyError(TrackType::kVideoTrack); You must notify for the error, even if there's no pending promise. Remove the test. This will also automatically resolve the promise @@ +556,5 @@ > + if (!mVideo.mPromise.IsEmpty()) { > + NotifyError(TrackType::kVideoTrack); > + } > + > + if (!mAudio.mPromise.IsEmpty()) { same here. @@ +562,5 @@ > + } > +} > + > +bool > +MediaFormatReader::UpdateIfStreamSourceIDChange(TrackType aTrack) you don't need this function. @@ +683,5 @@ > NS_WARNING("RequestVideoData on shutdown MediaFormatReader!"); > return VideoDataPromise::CreateAndReject(CANCELED, __func__); > } > > + if (!EnsureDecodersCreated()) { It seems to me that it would be preferable to ensure that decoders are created in DecodeDemuxedSamples() ; that way we have one code path where we can create decoders. @@ +702,5 @@ > nsRefPtr<VideoDataPromise> p = mVideo.mPromise.Ensure(__func__); > + > + // An update will be scheduled once the initialization promise is resolved. > + if(EnsureDecodersInitialized()) { > + ScheduleUpdate(TrackType::kVideoTrack); With creation and initialization done in DecodeDemuxedSample this also no longer necessary. All the work is done through Update() which is how I first intended things to be. Same in RequestAudioData() @@ +788,3 @@ > NS_WARNING("Error constructing decoders"); > return AudioDataPromise::CreateAndReject(DECODE_ERROR, __func__); > } remove @@ +793,5 @@ > + > + // An update will be scheduled once the initialization promise is resolved. > + if(EnsureDecodersInitialized()) { > + ScheduleUpdate(TrackType::kAudioTrack); > + } always schedule the update. @@ +1032,2 @@ > LOG("Unable to re-create decoder, aborting"); > NotifyError(aTrack); return; no need for else. otherwise you get the log that the decoder got created, when really, it hasn't ::: dom/media/MediaFormatReader.h @@ +104,5 @@ > void NotifyDemuxer(uint32_t aLength, int64_t aOffset); > void ReturnOutput(MediaData* aData, TrackType aTrack); > > + bool EnsureDecodersCreated(); > + // It returns true when decoders are initialized. False when there is pending when all decoders are initialized. ::: dom/media/fmp4/MP4Decoder.cpp @@ +287,5 @@ > nsRefPtr<MediaDataDecoder> decoder(CreateTestH264Decoder(aBackend, config)); > if (!decoder) { > return false; > } > + // XXX is it necessary to initialize before calling IsHardwareAccelerated()? if you're ensure, disable this code alltogether, and create a new patch for it, ask cpearce for review.

Comment on attachment 8631509 [details] [diff] [review] pdm_init_by_promise_h264converter Review of attachment 8631509 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/wrappers/H264Converter.cpp @@ +62,5 @@ > } > } > + > + if (mDecoderInitializing) { > + mMediaRawSamples.AppendElement(aSample); I don't see how you could ever be called with more than one sample here, ever. This actually indicates to me a bug in the H264Converter, in that it should request more data from the reader until it has seen a SPS and the decoders doesn't handle annexB. So right now H264Converter only properly handle cases where the SPS is found in the first MediaRawData received. need to open a new bug. @@ +178,5 @@ > +H264Converter::OnDecoderInitDone(const TrackType aTrackType) > +{ > + for (uint32_t i = 0 ; i < mMediaRawSamples.Length(); i++) { > + if (NS_FAILED(mDecoder->Input(mMediaRawSamples[i]))) { > + mCallback->Error(); It's up to MediaDataDecoder to call Error() if an error occurred. Should never happen, but I can understand why you would put it... maybe it make the code clearer to understand. Up to you to remove or keep

Comment on attachment 8632713 [details] [diff] [review] pdm_init_by_promise Review of attachment 8632713 [details] [diff] [review]: ----------------------------------------------------------------- It seems that you mistakenly removed way too much code. Change of resolution or audio content will not work anymore. ::: dom/media/MediaFormatReader.cpp @@ +535,5 @@ > + } > +} > + > +void > +MediaFormatReader::OnDecoderInitFailed(MediaDataDecoder::DecoderFailureReason aReason) Can't you pass the list of failed track instead? @@ +543,5 @@ > + > + mMetadataPromise.RejectIfExists(ReadMetadataFailureReason::METADATA_ERROR, __func__); > + > + NotifyError(mVideo.mPromise.IsEmpty() ? TrackType::kAudioTrack > + : TrackType::kVideoTrack); But say both failed to initialised. you're only going to set it to true. those errors are fatal anyway. so you can do: NotifyError(TrackType::kAudioTrack); NotifyError(TrackType::kVideoTrack); @@ +952,5 @@ > Flush(aTrack); > decoder.mDecoder->Shutdown(); > decoder.mDecoder = nullptr; > + decoder.mQueuedSamples.MoveElementsFrom(samples); > + return; And ... hmmm.. Where did all this code go ??? It's not going to work without it.

(In reply to Jean-Yves Avenard [:jya] from comment #17) > Comment on attachment 8632713 [details] [diff] [review] > pdm_init_by_promise > > Review of attachment 8632713 [details] [diff] [review]: > ----------------------------------------------------------------- > > It seems that you mistakenly removed way too much code. > Change of resolution or audio content will not work anymore. > > ::: dom/media/MediaFormatReader.cpp > @@ +535,5 @@ > > + } > > +} > > + > > +void > > +MediaFormatReader::OnDecoderInitFailed(MediaDataDecoder::DecoderFailureReason aReason) > > Can't you pass the list of failed track instead? The AllPromiseType returns one reject value only [1], it can't pass the list of failed track. Another idea is to separate the the audio/video init promise instead of an single all promise, it can solve this problem. The All promise also causes multiple requests problem (because audio and video could be requested separately), we can't use one All promise request holder to hold them. So I plan to rewrite this part to use audio and video init promise. [1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaPromise.h#209

Nah, I think using a All promise is much more elegant. And in any case, notifying just audio or just video is fine as an error is fatal (for now)

Comment on attachment 8634562 [details] [diff] [review] pdm_init_by_promise Review of attachment 8634562 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. looks very good. It needs rebase. In your rebase, you'll need the following change to MediaFormatReader::DrainDecoder() This currently doesn't work for me ; due to my machine not supporting VDA and the patch #2 doesn't handle this properly ::: dom/media/MediaFormatReader.cpp @@ +100,5 @@ > > mDemuxerInitRequest.DisconnectIfExists(); > mSeekPromise.RejectIfExists(NS_ERROR_FAILURE, __func__); > mSkipRequest.DisconnectIfExists(); > + mDecodersInitRequest.DisconnectIfExists(); please move up, just below mDemuxerInitRequest (so the promise appear in the logical order in which they are going to be used). @@ +493,5 @@ > + if (mDecodersInitRequest.Exists()) { > + return false; > + } > + > + bool init = (mVideo.mDecoder || mAudio.mDecoder); I don't think you need this init variable. Instead MOZ_ASSERT(mVideo.mDecoder || mAudio.mDecoder); as really, it's a bug if we get there without a decoder created. @@ +518,5 @@ > + LOG("init decoders: audio: %p, audio init: %d, video: %p, video init: %d", > + mAudio.mDecoder.get(), mAudio.mDecoderInitialized, > + mVideo.mDecoder.get(), mVideo.mDecoderInitialized); > + > + // Return false if any decoder is under initialization or decoder is not allocated. decoders not being allocated, can't happen. As EnsureDecodersCreated() would have returned an error before that. @@ +519,5 @@ > + mAudio.mDecoder.get(), mAudio.mDecoderInitialized, > + mVideo.mDecoder.get(), mVideo.mDecoderInitialized); > + > + // Return false if any decoder is under initialization or decoder is not allocated. > + return init; return !!promises.Length() @@ +528,5 @@ > +{ > + MOZ_ASSERT(OnTaskQueue()); > + mDecodersInitRequest.Complete(); > + > + for (uint32_t i = 0; i < aTrackTypes.Length(); i++) { for (const auto& track : aTrackTypes) { auto& decoder = GetDecoderData(track); @@ +541,5 @@ > + nsRefPtr<MetadataHolder> metadata = new MetadataHolder(); > + metadata->mInfo = mInfo; > + metadata->mTags = nullptr; > + mMetadataPromise.Resolve(metadata, __func__); > + return; no need for this return @@ +551,5 @@ > +{ > + MOZ_ASSERT(OnTaskQueue()); > + mDecodersInitRequest.Complete(); > + > + LOG("failed to init decoder"); I'd use NS_WARNING() as it's serious enough. and Failed (uppercase F) @@ +932,5 @@ > if (decoder.mQueuedSamples.IsEmpty()) { > return; > } > > + if (!EnsureDecodersCreated()) { We've lost the: NS_WARNING("Error constructing decoders"); please add it again. ::: dom/media/MediaFormatReader.h @@ +227,5 @@ > bool mWaitingForData; > bool mReceivedNewData; > bool mDiscontinuity; > + // False when decoder is created. True when decoder Init() promise is resolved. > + bool mDecoderInitialized; please move this line 248 (once rebased) just prior the declaration of bool mOutputRequested ; so that all MediaDataDecoder related members are in the same place

Comment on attachment 8631510 [details] [diff] [review] pdm_init_by_promise_platform Review of attachment 8631510 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/apple/AppleVDADecoder.cpp @@ +518,5 @@ > nsRefPtr<AppleVDADecoder> decoder = > new AppleVDADecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > + > + // XXX it needs to find another way to ensure the decoder supporting hardware > + // acceleration. This must be done. On machines not supporting VDA, as it will always fail, playback won't start.

Fix machines with no VDA support ; this will do the trick.

Comment on attachment 8631510 [details] [diff] [review] pdm_init_by_promise_platform r=me if you merge with the VDA fix I attached.

Comment on attachment 8634584 [details] [diff] [review] 0001-fix-vda.patch Review of attachment 8634584 [details] [diff] [review]: ----------------------------------------------------------------- oh maybe you could put this as bug 1179681. Or close bug 1179681, and merge it here.

Comment on attachment 8636422 [details] [diff] [review] pdm_init_by_promise_webm Review of attachment 8636422 [details] [diff] [review]: ----------------------------------------------------------------- You could make all of this much simpler by only dealing with IntelWebMVideoDecoder. please remove WebMReader::ReadMetadata ::: dom/media/webm/WebMReader.cpp @@ +293,2 @@ > nsresult WebMReader::ReadMetadata(MediaInfo* aInfo, > MetadataTags** aTags) Please merge ReadMetadata and AsyncReadMetadata so you only have AsyncReadMetadata : you can use a private member to replace ReadMetadata ::: dom/media/webm/WebMReader.h @@ +49,5 @@ > // Class to handle various audio decode paths > class WebMAudioDecoder > { > public: > + virtual nsRefPtr<InitPromise> Init() = 0; you could do without for simplicity sake. Though all this code is going away soon, so it doesn't really matter one way or the other. @@ +90,5 @@ > return mHasVideo; > } > > + virtual nsRefPtr<MetadataPromise> AsyncReadMetadata() override; > + trailing space @@ +95,2 @@ > virtual nsresult ReadMetadata(MediaInfo* aInfo, > MetadataTags** aTags) override; if you implement AsyncReadMetadata you don't want to also override ReadMetadata() as we should never get there.

(In reply to Jean-Yves Avenard [:jya] from comment #33) > Comment on attachment 8636422 [details] [diff] [review] > pdm_init_by_promise_webm > > Review of attachment 8636422 [details] [diff] [review]: > ----------------------------------------------------------------- > > You could make all of this much simpler by only dealing with > IntelWebMVideoDecoder. > > please remove WebMReader::ReadMetadata > > ::: dom/media/webm/WebMReader.cpp Thanks for quick review! I'll address them in next patch.

Comment on attachment 8636421 [details] [diff] [review] pdm_init_by_promise_mp4decoder Review of attachment 8636421 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/MP4Decoder.cpp @@ +274,5 @@ > nsRefPtr<MediaDataDecoder> decoder( > platform->CreateDecoder(aConfig, nullptr, nullptr, aBackend, nullptr)); > if (!decoder) { > return nullptr; > } Unfortunately, this is not going to work. Here we are initializing the WMF decoder, to verify that all libraries etc are installed. So although you moved all the init into CreateDecoder(), you don't provide a way to check that the init was successful, except by calling Init(), which we're no longer doing here. We need to know whether WMF initialized successfully here; this is how we determine whether WMF is going to work or not. One easy solution here would be for WMFDecoderModule::CreateDecoder() to initialize the decoder synchronously and return nullptr if that fails, and rely on the null check here to detect failure.

Platform patch needs rebasing.

Rebase.

will commit shortly

is causing bustage in static analysis: https://treeherder.mozilla.org/logviewer.html#?job_id=12721968&repo=mozilla-inbound

https://hg.mozilla.org/mozilla-central/rev/ac6d0b0befb2 https://hg.mozilla.org/mozilla-central/rev/ed7bf9f0cc92 https://hg.mozilla.org/mozilla-central/rev/7042219cf7f3