Closed
Bug 1214967
Opened 9 years ago
Closed 9 years ago
Fatal assert creating GMP decoder with e10s enabled
Categories
(Core :: Audio/Video: Playback, defect, P1)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(2 files, 1 obsolete file)
8.38 KB,
patch
|
billm
:
review+
jwwang
:
review+
|
Details | Diff | Splinter Review |
6.94 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
On pearce.org.nz/h, with e10s enabled.
xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::GetPluginVersionForAPI(const nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aHasPlugin, nsACString_internal & aOutVersion) Line 127 C++
xul.dll!mozilla::gmp::GeckoMediaPluginService::HasPluginForAPI(const nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aOutHavePlugin) Line 564 C++
xul.dll!mozilla::GMPDecoderModule::SupportsMimeType(const nsACString_internal & aMimeType, const mozilla::Maybe<nsCString> & aGMP) Line 154 C++
> xul.dll!mozilla::GMPDecoderModule::SupportsMimeType(const nsACString_internal & aMimeType) Line 163 C++
xul.dll!mozilla::PDMFactory::GetDecoder(const nsACString_internal & aMimeType) Line 275 C++
xul.dll!mozilla::PDMFactory::CreateDecoder(const mozilla::TrackInfo & aConfig, mozilla::FlushableTaskQueue * aTaskQueue, mozilla::MediaDataDecoderCallback * aCallback, mozilla::layers::LayersBackend aLayersBackend, mozilla::layers::ImageContainer * aImageContainer) Line 138 C++
xul.dll!mozilla::MediaFormatReader::EnsureDecodersCreated() Line 391 C++
xul.dll!mozilla::MediaFormatReader::OnDemuxerInitDone(nsresult __formal) Line 349 C++
xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::InvokeCallbackMethod<mozilla::MediaFormatReader,void (__thiscall mozilla::MediaFormatReader::*)(enum nsresult),enum nsresult const &>(mozilla::MediaFormatReader * aThisVal, void (nsresult) * aMethod, const nsresult & aValue) Line 437 C++
xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::MethodThenValue<mozilla::MediaFormatReader,void (__thiscall mozilla::MediaFormatReader::*)(enum nsresult),void (__thiscall mozilla::MediaFormatReader::*)(enum mozilla::DemuxerFailureReason)>::DoResolveOrRejectInternal(const mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ResolveOrRejectValue & aValue) Line 486 C++
xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::DoResolveOrReject(const mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ResolveOrRejectValue & aValue) Line 386 C++
xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::ResolveOrRejectRunnable::Run() Line 320 C++
xul.dll!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() Line 183 C++
xul.dll!mozilla::TaskQueue::Runner::Run() Line 171 C++
xul.dll!nsThreadPool::Run() Line 230 C++
Assignee | ||
Comment 2•9 years ago
|
||
This is a <video> playback issue, not a WebRTC related one. I'll take it.
Flags: needinfo?(mreavy)
Assignee | ||
Comment 3•9 years ago
|
||
The problem here is that we're calling the main thread only GeckoMediaPluginServiceChild::GetPluginVersionForAPI() to ask the chrome process if a GMP is supported from the decode thread. Presumably that's main thread only because it uses PContent to talk to the chrome process. A simple fix is to build a lookup table of the GMPs that we'll use for decoding in GMPDecoderModule::Init(), which runs on the main thread, and whenever that changes, update the table.
Assignee | ||
Comment 4•9 years ago
|
||
* Add a table of the GMP codecs we expect to use. This is on the main thread, so GMPService::HasPluginForAPI() works. * Implement GMPDecoderModule::SupportsMimeType() by consulting the the table. Updating the table to come in the next patch.
Attachment #8674653 -
Flags: review?(jwwang)
Assignee | ||
Comment 5•9 years ago
|
||
* Dispatch "gmp-changed" observer service notification whenever GMPs added or removed. This corresponds to the GMPProvider at startup, or if the user enables/disables a GMP via the about:addons>plugins page. * Proxy the "gmp-changed" observer service notification over from chrome to content process via PContent. r? jwwang for GMPServiceParent.cpp change. r? billm for PContent change.
Attachment #8674655 -
Flags: review?(wmccloskey)
Attachment #8674655 -
Flags: review?(jwwang)
Comment 6•9 years ago
|
||
Comment on attachment 8674653 [details] [diff] [review] Patch 1: Add lookup table of GMP codecs Review of attachment 8674653 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp @@ +207,5 @@ > + const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm"); > + const bool isH264 = aMimeType.EqualsLiteral("video/avc") || > + aMimeType.EqualsLiteral("video/mp4"); > + > + for (GMPCodecs& gmp : sGMPCodecs) { Is there any synchronization to ensure changes of sGMPCodecs is visible to other threads and no race in reading sGMPCodecs here while it is being updated on the main thread? @@ +220,2 @@ > } > + space.
Updated•9 years ago
|
Attachment #8674655 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #6) > Comment on attachment 8674653 [details] [diff] [review] > Patch 1: Add lookup table of GMP codecs > > Review of attachment 8674653 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp > @@ +207,5 @@ > > + const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm"); > > + const bool isH264 = aMimeType.EqualsLiteral("video/avc") || > > + aMimeType.EqualsLiteral("video/mp4"); > > + > > + for (GMPCodecs& gmp : sGMPCodecs) { > > Is there any synchronization to ensure changes of sGMPCodecs is visible to > other threads and no race in reading sGMPCodecs here while it is being > updated on the main thread? No. I don't think it's necessary since GMPs won't be aggressively switching off and on again... I can change GMPCodec::mHas* into atomics if you like, but it seems like overkill.
Comment 8•9 years ago
|
||
For correctness, the whole table must be updated in an indivisible transaction so GMPDecoderModule::SupportsMimeType() will never see the table in an inconsistent state. Failing to do so might lead to subtle bugs which rarely happen. So atomics doesn't fix that problem. You will need a mutex for that.
Comment 9•9 years ago
|
||
Come to think about it, if a user asks supported codec while GMP add/remove is in action, this is inherently a race and the result is indeterministic. So this should be probably fine.
Updated•9 years ago
|
Attachment #8674653 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 10•9 years ago
|
||
With StaticMutex.
Attachment #8674653 -
Attachment is obsolete: true
Attachment #8674670 -
Flags: review?(jwwang)
Updated•9 years ago
|
Attachment #8674670 -
Flags: review?(jwwang) → review+
Comment on attachment 8674655 [details] [diff] [review] Patch 2: Update GMP PDM's codec table on GMP enable/disable Review of attachment 8674655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +3295,5 @@ > } > } > } > + else if (!strcmp(aTopic, "gmp-changed")) { > + unused << SendNotifyGMPsChanged(); We shouldn't do this if IsDestroyed().
Attachment #8674655 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #11) > Comment on attachment 8674655 [details] [diff] [review] > Patch 2: Update GMP PDM's codec table on GMP enable/disable > > Review of attachment 8674655 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/ContentParent.cpp > @@ +3295,5 @@ > > } > > } > > } > > + else if (!strcmp(aTopic, "gmp-changed")) { > > + unused << SendNotifyGMPsChanged(); > > We shouldn't do this if IsDestroyed(). Thanks. I see other messages being sent without checking IsDestroyed(), should we file a follow-up to add IsDestroyed() checks to them too?
Flags: needinfo?(wmccloskey)
I looked at this a bit more and I guess the check isn't needed in this particular case. For non-toplevel actors, it's possible that the MessageChannel will have been destroyed and we'll segfault if we Send after ActorDestroy. But a toplevel actor (like ContentParent) owns its channel, so that won't happen. In addition, Send will call ProcessingError(MessageDropped) if the connection is closed. But ContentParent ignores that error: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1940 It's still good practice to do the check, but I guess there's no harm in missing it on ContentParent.
Flags: needinfo?(wmccloskey)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6b752acbd0 https://hg.mozilla.org/integration/mozilla-inbound/rev/120b53ea0f41
Comment 15•9 years ago
|
||
The only thing more unwelcome than being backed out for an incomprehensible and seemingly-unrelated b2g failure like https://treeherder.mozilla.org/logviewer.html#?job_id=15796569&repo=mozilla-inbound is being backed out for etc. in a suite which takes two bloody hours to run. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4eaf357c0a
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 16•9 years ago
|
||
I've wasted too much time trying to debug this on try and I can't get B2G emulator to build locally. So I'll just disable the "gmp-changed" observer service notification on B2G, and get this landed and move on. We're not interested in using GMPs for non-EME decoding on B2G anyway, and not expecting GMPs to change at runtime on B2G at this stage. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ba8faaf735f
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81edda656162 https://hg.mozilla.org/integration/mozilla-inbound/rev/285de7bf33e3
Comment 18•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81edda656162 https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 19•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/81edda656162 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/285de7bf33e3
status-b2g-v2.5:
--- → fixed
Comment 20•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•