Status
()
People
(Reporter: cpearce, Assigned: cpearce)
Tracking
Firefox Tracking Flags
(e10s+, firefox45 fixed)
Details
Attachments
(2 attachments, 1 obsolete attachment)
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•3 years ago
|
||
This is a <video> playback issue, not a WebRTC related one. I'll take it.
Flags: needinfo?(mreavy)
(Assignee) | ||
Comment 3•3 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•3 years ago
|
||
Created attachment 8674653 [details] [diff] [review] Patch 1: Add lookup table of GMP codecs * 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•3 years ago
|
||
Created attachment 8674655 [details] [diff] [review] Patch 2: Update GMP PDM's codec table on GMP enable/disable * 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•3 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•3 years ago
|
Attachment #8674655 -
Flags: review?(jwwang) → review+
(Assignee) | ||
Comment 7•3 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•3 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•3 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•3 years ago
|
Attachment #8674653 -
Flags: review?(jwwang) → review+
(Assignee) | ||
Comment 10•3 years ago
|
||
Created attachment 8674670 [details] [diff] [review] Patch 1v2: Add lookup table of GMP codecs With StaticMutex.
Attachment #8674653 -
Attachment is obsolete: true
Attachment #8674670 -
Flags: review?(jwwang)
Updated•3 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•3 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•3 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6b752acbd0 https://hg.mozilla.org/integration/mozilla-inbound/rev/120b53ea0f41
Comment 15•3 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•3 years ago
|
Priority: -- → P1
Updated•3 years ago
|
tracking-e10s: --- → +
(Assignee) | ||
Comment 16•3 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•3 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81edda656162 https://hg.mozilla.org/integration/mozilla-inbound/rev/285de7bf33e3
Comment 18•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81edda656162 https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 19•3 years ago
|
||
bugherderuplift |
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•3 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
•