Closed
Bug 1151611
Opened 10 years ago
Closed 10 years ago
Expose DXVA status in about:support and crashes
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: mattwoodrow)
References
Details
Attachments
(1 file)
(deleted),
patch
|
cpearce
:
review+
Felipe
:
review+
|
Details | Diff | Splinter Review |
This would be helpful for diagnosing problems.
Assignee | ||
Comment 1•10 years ago
|
||
A few thoughts about this:
We can't really tell if DXVA will be used until we have an actual video playing, and we've attempted to initialize the decoder.
Possible ideas:
* Add about:media to all versions of firefox, and display DXVA status for any currently playing videos.
* Attempt to initialize a dummy DXVA decoder when we open about:support and check if that succeeds.
* Make about:support report the status of the prefs/blacklisting controlling DXVA, and don't worry about checking if the hardware supports it.
* Have a 'ever initialized' value reported in about:support that will start returning true after the first DXVA video is played.
Any thoughts Anthony?
Blocks: 1151721
Flags: needinfo?(ajones)
Comment 2•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> * Add about:media to all versions of firefox, and display DXVA status for
> any currently playing videos.
Gavin won't have a bar of it.
> * Attempt to initialize a dummy DXVA decoder when we open about:support and
> check if that succeeds.
This seems a good option on the basis that it will give us the most accurate answer.
We may also want to record information about fallbacks. Perhaps we could record the success/failure of dxva in a string of 1s and 0s so we can work out a percentage of time DXVA gets used. It may be worth splitting this between different video resolutions.
Perhaps: DXVA 2160=0% 1080=90% 720=100% <=480=100%
Flags: needinfo?(ajones)
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)
>
> > * Attempt to initialize a dummy DXVA decoder when we open about:support and
> > check if that succeeds.
>
> This seems a good option on the basis that it will give us the most accurate
> answer.
Ok.
>
> We may also want to record information about fallbacks. Perhaps we could
> record the success/failure of dxva in a string of 1s and 0s so we can work
> out a percentage of time DXVA gets used. It may be worth splitting this
> between different video resolutions.
>
> Perhaps: DXVA 2160=0% 1080=90% 720=100% <=480=100%
Why would initialization succeed for a given resolution some of the time but not others? With the shared decoders we also just initialize once, and then reconfigure for different resolutions.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8594615 -
Flags: review?(cpearce)
Comment 5•10 years ago
|
||
Comment on attachment 8594615 [details] [diff] [review]
Add DXVA status to about:support
Review of attachment 8594615 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/fmp4/MP4Reader.cpp
@@ +91,5 @@
> + nsRefPtr<PlatformDecoderModule> platform = PlatformDecoderModule::Create();
> +
> + nsRefPtr<MediaDataDecoder> decoder =
> + platform->CreateDecoder(config, nullptr, nullptr, aBackend, nullptr);
> + nsresult rv = decoder->Init();
I'm a little wary here of calling WMFVideoMFTManager::Init() on the main thread, as it calls WMFVideoMFTManager::InitializeDXVA() which does a sync dispatch to the main thread, and it looks like nsThread just spins the event loop in that case. So we might get unexpected results there.
So I think you should add a check to WMFVideoMFTManager::InitializeDXVA() that tests if we're on the main thread and just calls the runnable if so immediately, rather than doing an unnecessary sync dispatch to the main thread.
::: dom/media/fmp4/wmf/WMFVideoMFTManager.cpp
@@ -78,5 @@
> , mLayersBackend(aLayersBackend)
> // mVideoStride, mVideoWidth, mVideoHeight, mUseHwAccel are initialized in
> // Init().
> {
> - NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");
The same assertion was cargo-culted into GonkVideoDecoderManager's constructor... So you should probably remove it from there too.
::: toolkit/modules/tests/browser/browser_Troubleshoot.js
@@ +198,5 @@
> },
> windowLayerManagerRemote: {
> type: "boolean",
> },
> + supportsHardwareH264: {
This doesn't look like it's indented correctly.
Also, I'm not sure how qualified I am to review changes to this code...
Attachment #8594615 -
Flags: review?(cpearce) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8594615 -
Flags: review?(felipc)
Comment 6•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Why would initialization succeed for a given resolution some of the time but
> not others? With the shared decoders we also just initialize once, and then
> reconfigure for different resolutions.
The black frame detection on AMD will cause us to fall back. It happens more on higher resolutions. Maybe we can track this better through telemetry.
Comment 7•10 years ago
|
||
Comment on attachment 8594615 [details] [diff] [review]
Add DXVA status to about:support
Review of attachment 8594615 [details] [diff] [review]:
-----------------------------------------------------------------
The front-end changes look fine. I'm a bit uneasy about knowing that opening about:support will trigger this less-common path of initializing the platform decoder in the main thread.
Also, shouldn't it be possible to cache an answer if h264 decoding has already happened during this session? (instead of trying to instantiate the test video). Is that worth it?
::: toolkit/modules/Troubleshoot.jsm
@@ +312,5 @@
> getInterface(Ci.nsIDOMWindowUtils);
> try {
> data.windowLayerManagerType = winUtils.layerManagerType;
> data.windowLayerManagerRemote = winUtils.layerManagerRemote;
> + data.supportsHardwareH264 = winUtils.supportsHardwareH264Decoding;
nit: tabs
Attachment #8594615 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to :Felipe Gomes from comment #7)
>
> Also, shouldn't it be possible to cache an answer if h264 decoding has
> already happened during this session? (instead of trying to instantiate the
> test video). Is that worth it?
>
We could do that, but we'd still need this code for the cases where a video hasn't been watched before. Given that, I don't think it's worth adding the extra complexity.
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7362533abe5a
https://hg.mozilla.org/mozilla-central/rev/fafcc7f4f663
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•