When the GPU process crashes we sometimes fall back to software compositing in the UI process. MediaFormatReader cached a pointer to the LayerManager, so won't detect this change. We need to notify it when we have a new LayerManager.

Comment on attachment 8808022 [details] [diff] [review] notify-mfr Review of attachment 8808022 [details] [diff] [review]: ----------------------------------------------------------------- If I understand correctly when the GPU MediaDataDecoder (MDD) crashes, it will call the MediaFormatReader error callback with NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER. This will cause the MDD to be shutdown and a new one will be recreated. In parallel, this may cause the LayerManager to change from HW to SW one, and then will call NotifyOwnerActivityChanged(). But by that time the MDD may have already been recreated, still with the old cached LayerManager. So we would have to wait for the GPU decoder to crash again so we recreate one, this time using the SW decoder. It seems to me that we need a way to synchronise the notification with NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER and NotifyOwnerActivityChanged(). Adding :jwwang for review, he may have more to say. But at this stage, I'd like to remove cross-threading methods like this and instead use MediaEvents ::: dom/media/MediaFormatReader.cpp @@ +556,5 @@ > + // We might have a new layer manager, make sure we update our > + // pointer. We assume we'll get an error (if necessary), recreate > + // our decoders and those will use the updated layers backend. > + MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); > + InitLayersBackendType(); I have my doubt in the thread safetiness of InitLayersBackendType() and in particular: mKnowsCompositor = layerManager->AsShadowForwarder(); This would be modified on the main thread where it's likely to coincide right when the GPU MediaDataDecoder has been shutdown and we're in the middle of re-creating one. as such, accessing mKnowsCompositor is no longer thread-safe. Previously, this was only modified during Init which happened during construction, prior the MFR object being used. ClientLayerManager::AsShadowForwarder() doesn't appear thread safe to me either. One way to remove the thread-safety issue in accessing mKnowsCompositor would be to use MediaEvent instead. Not only is this more in-line with our threading model (we aim to remove all cross-thread call such as MediaDecoder::NotifyOwnerActivityChanged) So have a MediaEvent/MediaEventListener between the MediaDecoder and the MediaDecoderReader. You don't need to go through the MediaDecoderStateMachine.

(In reply to Matt Woodrow (:mattwoodrow) from comment #0) > MediaFormatReader cached a pointer to the LayerManager, so won't detect this > change. We need to notify it when we have a new LayerManager. What will happen if MediaFormatReader keeps using the old LayerManager?

Comment on attachment 8808022 [details] [diff] [review] notify-mfr Review of attachment 8808022 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaFormatReader.cpp @@ +556,5 @@ > + // We might have a new layer manager, make sure we update our > + // pointer. We assume we'll get an error (if necessary), recreate > + // our decoders and those will use the updated layers backend. > + MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); > + InitLayersBackendType(); http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/MediaFormatReader.cpp#429 mKnowsCompositor is used by MFR only when creating a new MediaDataDecoder. So changing mKnowsCompositor won't take effect until a new MDD is created, right?

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2) > (In reply to Matt Woodrow (:mattwoodrow) from comment #0) > > MediaFormatReader cached a pointer to the LayerManager, so won't detect this > > change. We need to notify it when we have a new LayerManager. > > What will happen if MediaFormatReader keeps using the old LayerManager? then it will create a HW decoder instead of SW.

(In reply to Jean-Yves Avenard [:jya] from comment #4) > (In reply to JW Wang [:jwwang] [:jw_wang] from comment #2) > > (In reply to Matt Woodrow (:mattwoodrow) from comment #0) > > > MediaFormatReader cached a pointer to the LayerManager, so won't detect this > > > change. We need to notify it when we have a new LayerManager. > > > > What will happen if MediaFormatReader keeps using the old LayerManager? > > then it will create a HW decoder instead of SW. What's the consequence of a HW decoder messing with a basic compositor?

Comment on attachment 8808022 [details] [diff] [review] notify-mfr r+ for the non dom/media stuff. Looks like EnumerateActivityObservers is called somewhat random times. Would be nice to document somewhere what it is trying to capture.

(In reply to Jean-Yves Avenard [:jya] from comment #1) > Comment on attachment 8808022 [details] [diff] [review] > notify-mfr > > Review of attachment 8808022 [details] [diff] [review]: > ----------------------------------------------------------------- > > If I understand correctly when the GPU MediaDataDecoder (MDD) crashes, it > will call the MediaFormatReader error callback with > NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER. This will cause the MDD to be shutdown > and a new one will be recreated. > In parallel, this may cause the LayerManager to change from HW to SW one, > and then will call NotifyOwnerActivityChanged(). > But by that time the MDD may have already been recreated, still with the old > cached LayerManager. So we would have to wait for the GPU decoder to crash > again so we recreate one, this time using the SW decoder. > > It seems to me that we need a way to synchronise the notification with > NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER and NotifyOwnerActivityChanged(). > > Adding :jwwang for review, he may have more to say. > > But at this stage, I'd like to remove cross-threading methods like this and > instead use MediaEvents We actually intentionally delay the reporting of NS_ERROR_DOM_MEDIA_NEW_NEW_DECODER so that it happens after NotifyOwnerActivityChanged has finished. I was thinking that we needed to update the LayerManager on the main thread so that there wasn't a race, but as long as we put the event into the TaskQueue before the error callback (which also goes into the same task queue) then we should be fine. I'll look into MediaEvents.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2) > (In reply to Matt Woodrow (:mattwoodrow) from comment #0) > > MediaFormatReader cached a pointer to the LayerManager, so won't detect this > > change. We need to notify it when we have a new LayerManager. > > What will happen if MediaFormatReader keeps using the old LayerManager? If we're still using the old LayerManager then RemoteDecoderModule::CreateVideoDecoder will still think we're connected to the GPU process, so it will allow creation of a RemoteVideoDecoder, rather than returning nullptr and falling back to WMFDecoderModule. Since we don't actually have the GPU process any more, when RemoteVideoDecoder dispatches VideoDecoderChild::InitIPDL to the video thread, that will fail (the manager check will fail) and we don't initialize mIPDLSelfRef. Then when we dispatch Init() to RemoteVideoDecoder/VideoDecoderChild it will fail immediately (since we failed InitIPDL), and we'll report NS_ERROR_DOM_MEDIA_NEED_NEW_DECODER back to MFR. MFR might try again, and if we never update the layer manager, then this could loop and never play the video. Note that my comments depend on bug 1315510 and bug 1315144 which haven't landed yet. (In reply to JW Wang [:jwwang] [:jw_wang] from comment #3) > Comment on attachment 8808022 [details] [diff] [review] > notify-mfr > > Review of attachment 8808022 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaFormatReader.cpp > @@ +556,5 @@ > > + // We might have a new layer manager, make sure we update our > > + // pointer. We assume we'll get an error (if necessary), recreate > > + // our decoders and those will use the updated layers backend. > > + MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); > > + InitLayersBackendType(); > > http://searchfox.org/mozilla-central/rev/ > f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/MediaFormatReader.cpp#429 > > mKnowsCompositor is used by MFR only when creating a new MediaDataDecoder. > So changing mKnowsCompositor won't take effect until a new MDD is created, > right? This depends on bug 1315510, but the idea is that *after* we've called NotifyOwnerActivityChanged we dispatch the Error(NS_DOM_MEDIA_NEED_NEW_DECODER) from the video thread -> MFR task queue which will force us to shutdown the old decoder and create a new one, using the new layer manager.

(In reply to Matt Woodrow (:mattwoodrow) from comment #8) > This depends on bug 1315510, but the idea is that *after* we've called > NotifyOwnerActivityChanged we dispatch the > Error(NS_DOM_MEDIA_NEED_NEW_DECODER) from the video thread -> MFR task queue > which will force us to shutdown the old decoder and create a new one, using > the new layer manager. It looks like a race to me. NS_DOM_MEDIA_NEED_NEW_DECODER is dispatched from the video thread to the MFR task queue while the new layer manager is dispatched from the main thread to the MFR task queue. How do we guarantee the later happens before the former?

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #10) > (In reply to Matt Woodrow (:mattwoodrow) from comment #8) > > This depends on bug 1315510, but the idea is that *after* we've called > > NotifyOwnerActivityChanged we dispatch the > > Error(NS_DOM_MEDIA_NEED_NEW_DECODER) from the video thread -> MFR task queue > > which will force us to shutdown the old decoder and create a new one, using > > the new layer manager. > > It looks like a race to me. NS_DOM_MEDIA_NEED_NEW_DECODER is dispatched from > the video thread to the MFR task queue while the new layer manager is > dispatched from the main thread to the MFR task queue. How do we guarantee > the later happens before the former? So firstly, it doesn't matter if it's a race, since if we create the new decoder *before* we update the LayerManager, then the new decoder will fail too, cause a second error, and by then the new LayerManager should be available. Secondly, NotifyActivityChanged is called on the main thread, and before we recreate the video manager (VideoDecoderManagerChild::InitForContent). By the time InitForContent is called, we should have already dispatched the new layer manager to the MFR task queue. InitForContent posts a task to the video thread, which calls VideoDecoderManagerChild::Open, and from there we dispatch all the errors from the previous manager. So those Error() calls are guaranteed to be in the MFR task queue after the new layer manager.

Sounds good to me if InitForContent also happens on the main thread.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #12) > Sounds good to me if InitForContent also happens on the main thread. Indeed it is. The core bit is here: http://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1225 The tabChild->ReinitRendering calls are what dispatch NotifyActivityChanged, and the VideoDecoderManagerChild::InitForContent will be called after that.

Comment on attachment 8808398 [details] [diff] [review] notify-mfr Review of attachment 8808398 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me for the media changes. ::: dom/media/AbstractMediaDecoder.h @@ +71,5 @@ > } > > + // Returns an event that will be notified when the owning document changes state > + // and we might have a new compositor. If this new compositor requires us to > + // recreate our decoders, then we expect the existing decoderis to return an s/decoderis/decoders/ ::: dom/media/MediaDecoder.cpp @@ +309,5 @@ > + nsContentUtils::LayerManagerForDocument(element->OwnerDoc()); > + NS_ENSURE_TRUE_VOID(layerManager); > + > + RefPtr<KnowsCompositor> knowsCompositor = layerManager->AsShadowForwarder(); > + mCompositorUpdatedEvent.Notify(knowsCompositor); Is it OK to notify the unchanged compositor again when NotifyOwnerActivityChanged() is called due to visibility changes?

Comment on attachment 8808398 [details] [diff] [review] notify-mfr Review of attachment 8808398 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the changes...

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e38e63ec11cc Notify MediaFormatReader when the compositor gets recreated. r=jya,smaug,jw_wang