Closed
Bug 1160914
Opened 9 years ago
Closed 9 years ago
[EME] Crash in VideoDecoder::SampleToVideoFrame()
Categories
(Core :: Audio/Video, defect, P2)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox37 | --- | unaffected |
firefox38 | --- | wontfix |
firefox38.0.5 | --- | wontfix |
firefox39 | --- | wontfix |
firefox40 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
17.71 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
5.26 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
Adobe reported that they are crashing in VideoDecoder::SampleToVideoFrame(). Call stack is: eme-adobe.dll!VideoDecoder::SampleToVideoFrame(IMFSample * aSample, GMPVideoi420Frame * aVideoFrame, int newWidth, int newHeight, int newStride) Line 673 C++ eme-adobe.dll!VideoDecoder::ReturnOutput(IMFSample * aSample, int newWidth, int newHeight, int newStride) Line 285 C++ eme-adobe.dll!gmp_task_args_m_4<VideoDecoder *,void (__thiscall VideoDecoder::*)(IMFSample *,int,int,int),wmf::CComPtr<IMFSample>,int,int,int>::Run() Line 371 C++ eme-adobe.dll!`VideoDecoder::MaybeRunOnMainThread'::`2'::MaybeRunTask::Run() Line 798 C++ The problem is I think that the main thread is spinning and processing events while waiting for a GMPVideoHost::CreateFrame() call to finish, and during that time a DecodingComplete() call comes in which deletes the VideoDecoder object, and then we deref free'd memory.
Assignee | ||
Comment 1•9 years ago
|
||
* Make VideoDecoder, AudioDecoder, and ClearKeySessionManager threadsafe refcounted. * Make the tasks that call into the VideoDecoder, AudioDecoder, and ClearKeySessionManager on the main thread hold a refcount, so that if the decoder is shutdown, the memory being used isn't freed until after the call exits. * Check return values of calls into GMPVideoHost or GMPVideoi420FrameImpl/GMPPlaneImpl so that we detect failure earlier. I can't reproduce any failures, but this should help...
Attachment #8600752 -
Flags: review?(edwin)
Attachment #8600752 -
Flags: review?(edwin) → review+
Comment 2•9 years ago
|
||
Fixing this crash in Adobe's CDM is a release blocker (for Adobe), but fixing this crash in the ClearKey CDM is not a Firefox release blocker.
status-firefox37:
--- → unaffected
status-firefox38:
--- → affected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Priority: -- → P2
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/000c1797e8a0 for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=9555980&repo=mozilla-inbound
Flags: needinfo?(cpearce)
Assignee | ||
Comment 5•9 years ago
|
||
Simple refcount-using-locks when <atomic> is not available: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab2710984cb
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c110ae720f
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fdcbf07ee6
Flags: needinfo?(cpearce)
Assignee | ||
Comment 8•9 years ago
|
||
We can't just #include <atomic>, as that breaks on our build machines. So implement threadsafe refcounting via locks where <atomic> doesn't work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=604b511fca6f
Attachment #8601371 -
Flags: review?(edwin)
Comment on attachment 8601371 [details] [diff] [review] Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available Review of attachment 8601371 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/gmp-clearkey/0.1/ClearKeySession.cpp @@ +22,5 @@ > > #include "gmp-api/gmp-decryption.h" > #include "Endian.h" > #include <assert.h> > +#include <string.h> Unnecessary, AFAICT. Debugging artefact? ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp @@ +23,5 @@ > #include "ClearKeyUtils.h" > #include "ClearKeyBase64.h" > #include "ArrayUtils.h" > #include <assert.h> > +#include <memory.h> Same here.
Attachment #8601371 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Edwin Flores [:eflores] [:edwin] from comment #9) > Comment on attachment 8601371 [details] [diff] [review] > Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic > is not easily available > > Review of attachment 8601371 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/gmp-clearkey/0.1/ClearKeySession.cpp > @@ +22,5 @@ > > > > #include "gmp-api/gmp-decryption.h" > > #include "Endian.h" > > #include <assert.h> > > +#include <string.h> > > Unnecessary, AFAICT. Debugging artefact? > > ::: media/gmp-clearkey/0.1/ClearKeyUtils.cpp > @@ +23,5 @@ > > #include "ClearKeyUtils.h" > > #include "ClearKeyBase64.h" > > #include "ArrayUtils.h" > > #include <assert.h> > > +#include <memory.h> > > Same here. These were required to fix non-unified build in my Linux VM.
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/44317690f61d https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4307edffc2
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44317690f61d https://hg.mozilla.org/mozilla-central/rev/0d4307edffc2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8600752 [details] [diff] [review] Patch: Make gmp-clearkey's decoders threadsafe refcounted Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Negligable. This fixes a crash in our our test CDM, so will help make our units tests more green. [Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests. [Risks and why]: Low; this doesn't touch code that we expect users to use. [String/UUID change made/needed]: None.
Attachment #8600752 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8601371 [details] [diff] [review] Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Negligable. This fixes a crash in our our test CDM, so will help make our units tests more green. [Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests. [Risks and why]: Low; this doesn't touch code that we expect users to use. [String/UUID change made/needed]: None.
Attachment #8601371 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 16•9 years ago
|
||
The patch here is based on top of Bug 1160321, so we should land that on beta 38 first.
Flags: needinfo?(cpearce)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
Comment 17•9 years ago
|
||
Bug 1160321 is still waiting for another patch to land (in bug 1164480) so let's wait to land them all together on 39.
Comment 18•9 years ago
|
||
Chris are you expecting this to uplift to 38.0.5?
Assignee | ||
Comment 19•9 years ago
|
||
No. I figured we should target EME fixes for 39. I'd like to uplift this, and all other EME uplift requests, in time to be in the first 39 beta build, so that when 39 hits the beta channel all fixes are in.
Flags: needinfo?(cpearce)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
Comment 20•9 years ago
|
||
Comment on attachment 8600752 [details] [diff] [review] Patch: Make gmp-clearkey's decoders threadsafe refcounted Approved for uplift to beta (39). Low risk change in test CDM.
Attachment #8600752 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•9 years ago
|
||
Comment on attachment 8601371 [details] [diff] [review] Patch 2: Use locks for gmp-clearkey threadsafe refcounting when std::atomic is not easily available Approved for uplift to beta (39) to improve EME test instability.
Attachment #8601371 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=653bd3d35f0c
Assignee | ||
Comment 23•9 years ago
|
||
I give up trying to rebase/uplift this; and since it only really affect our toy CDM, it's not a huge loss.
Flags: needinfo?(cpearce)
Updated•9 years ago
|
Attachment #8600752 -
Flags: approval-mozilla-beta+
Updated•9 years ago
|
Attachment #8601371 -
Flags: approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•