Closed Bug 1332234 Opened 7 years ago Closed 7 years ago

Add support for native AMD VP9 MFT

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files)

0001-Bug-1332234-Add-support-for-AMD-s-VP9-hardware-decod.patch
10.13 KB, patch
Details | Diff | Splinter Review
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
For technical reason, AMD VP9 hardware decoding isn't available through the official Microsoft VP9 MFT.

Instead they provide their own, which also happens to be more efficient.

As we currently only support VP9 hardware decoding via the Microsoft MFT, it means that users with an AMD card do not fully benefit from their hardware capabilities.

We should add support for it.
http://www.anandtech.com/show/10894/amd-delivers-crimson-relive-yearly-feature-update-for-radeon-gamers-and-professionals/6

"This is ultimately a software solution using the GPU, so it isn’t as power efficient as a dedicated hardware solution"

AMD does not have real fixed function VP9 hardware decoding on any GPU like Intel Kaby Lake or Nvidia Maxwell GM206 or Pascal. It's a hybrid/partial hardware decoding which everyone found out to be quite useless on Intel Broadwell & Skylake.

https://bugzilla.mozilla.org/show_bug.cgi?id=1225019

"The Intel VPx hardware decoder has some buggy drivers and can actually be slower than the ffvp9 software decoder."

http://forum.doom9.org/showthread.php?p=1790608#post1790608

"The problem is the framerate is very low 7 fps to 13 fps and the GPU utilization/clocks not very high."

It also doesn't work well even when tested on Chrome and it's not exposed properly when checked with DXVAChecker unless you know the registry hack for it.

Please proceed cautiously because hybrid/partial decoding can be worse than software decoding as seen from experience with Intel's hybrid/partial decoding.
Certainly not my experience using a RX-460 in Chrome. 60fps 4K VP9 with less than 5% CPU usage, super smooth.
While Firefox using ffvp9 was at over 60% and dropping frames.

Same with Kaby Lake prototype board provided by Intel using either Edge or Chrome.

The link you provided with the comment on the Intel VPX was for broadwell and skylake, neither of them have VP9 HW decoder, so isn't applicable. The code referred to has been removed from Firefox code base, we now only support hardware will full VP9 hardware decoding.
Comment on attachment 8897348 [details]
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.

https://reviewboard.mozilla.org/r/168658/#review174242

In general it looks fine, but I don't see why you can't use the actual program files path.

::: dom/media/platforms/wmf/WMFUtils.h:68
(Diff revision 1)
>  
> +// Will retrieve either CSIDL_PROGRAM_FILES or %ProgramW6432 if wow64_status
> +// returns true as per
> +// https://msdn.microsoft.com/library/windows/desktop/aa384274.aspx
> +// However, for now return a static path using the same drive as
> +// CSIDL_PROGRAM_FILES value.

I don't understand why you're not using the program files dir returned by the OS in this function?

::: dom/media/platforms/wmf/WMFUtils.h:69
(Diff revision 1)
> +// Will retrieve either CSIDL_PROGRAM_FILES or %ProgramW6432 if wow64_status
> +// returns true as per
> +// https://msdn.microsoft.com/library/windows/desktop/aa384274.aspx
> +// However, for now return a static path using the same drive as
> +// CSIDL_PROGRAM_FILES value.
> +nsString GetProgramFilePath();

GetProgramFile*s*Path()

Files with an 's'.

::: dom/media/platforms/wmf/WMFUtils.cpp:156
(Diff revision 1)
> +GetProgramFilePath()
> +{
> +  nsString path;
> +  wchar_t programPath[MAX_PATH + 1];
> +
> +  if (FAILED(SHGetFolderPath(NULL, CSIDL_PROGRAM_FILES, NULL,

MSDN says that SHGetFolderPath is deprecated, and that you should be using SHGetKnownFolderPath.

Note that you pass a PWSTR\* to SHGetKnownFolderPath() which you need to free.

  PWSTR path = nullptr;
  HRESULT hr = SHGetKnownFolderPath(FOLDERID_ProgramFiles, 0, 0, &path);
  // ... copy path
  CoTaskMemFree(path);

::: dom/media/platforms/wmf/WMFUtils.cpp:158
(Diff revision 1)
> +  nsString path;
> +  wchar_t programPath[MAX_PATH + 1];
> +
> +  if (FAILED(SHGetFolderPath(NULL, CSIDL_PROGRAM_FILES, NULL,
> +                             SHGFP_TYPE_CURRENT, programPath))) {
> +    return NS_LITERAL_STRING("C:\\Program Files");

I would assume that 32bit Firefox on 64bit Windows would need "Program Files (x86)", but 32bit Firefox on 32bit Windows would need just "Program Files".

So your default is wrong in the 32bit Firefox on 64bit Windows case, which is (currently) our most populous user base.

Anyway, I think if SHGetFolderPath fails to find program files, I think you should just give up on loading the AMD hardware decoder. There's likely bigger issues with the user's machine!

::: dom/media/platforms/wmf/WMFUtils.cpp:160
(Diff revision 1)
> +
> +  if (FAILED(SHGetFolderPath(NULL, CSIDL_PROGRAM_FILES, NULL,
> +                             SHGFP_TYPE_CURRENT, programPath))) {
> +    return NS_LITERAL_STRING("C:\\Program Files");
> +  }
> +  int drive = PathGetDriveNumber(programPath);

Why are you getting the ProgramFiles path from Windows, and then grabbing the drive, and then reconstructing a new path? Why can't you trust the value returned by Windows?

::: dom/media/platforms/wmf/WMFUtils.cpp:167
(Diff revision 1)
> +    // No path found, assume C:
> +    drive = L'C' - L'A';
> +  }
> +
> +  path.Append(L'A' + drive);
> +  path.Append(NS_LITERAL_STRING(":\\Program Files"));

It doesn't look like you're constructing a valid path here. It will be something like "A3:\\Program Files", which is not a valid Windows path format.

Why can't you just return the value of ShGetKnownFolderPath?

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:649
(Diff revision 1)
>    hr = mDecoder->GetOutputMediaType(outputType);
>    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
>  
>    if (mUseHwAccel && !CanUseDXVA(outputType)) {
>      mDXVAEnabled = false;
> -    // DXVA initialization actually failed, re-do initialisation.
> +    // DXVA initialization with current decoder actually failed,

We have initialization spelt with an 's' and a 'z' in the same sentance here.
Attachment #8897348 - Flags: review?(cpearce) → review-
Comment on attachment 8897348 [details]
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.

https://reviewboard.mozilla.org/r/168658/#review174242

Because the Program File path you need is *NOT* CSIDL_PROGRAM_FILES, but ProgramW6432.
That environment variable is only available on win7 and later (not vista).
If you use CLSID_ProgramFile, on 32 bits build in 64 bits env it's c:\Program files (x86).

I even put a comment two lines above explaining this

> I don't understand why you're not using the program files dir returned by the OS in this function?

because you can't.

It wouldn't work on 32 bits system depending on the wow64_status value

> MSDN says that SHGetFolderPath is deprecated, and that you should be using SHGetKnownFolderPath.
> 
> Note that you pass a PWSTR\* to SHGetKnownFolderPath() which you need to free.
> 
>   PWSTR path = nullptr;
>   HRESULT hr = SHGetKnownFolderPath(FOLDERID_ProgramFiles, 0, 0, &path);
>   // ... copy path
>   CoTaskMemFree(path);

SHGetKnownFolderPath is much more difficult to use (gotta worry about freeing it and so on), and SHGetFolderPath is what's used everywhere in our code.... So if we want to stop using deprecated code, better start fixing those other ones

> I would assume that 32bit Firefox on 64bit Windows would need "Program Files (x86)", but 32bit Firefox on 32bit Windows would need just "Program Files".
> 
> So your default is wrong in the 32bit Firefox on 64bit Windows case, which is (currently) our most populous user base.
> 
> Anyway, I think if SHGetFolderPath fails to find program files, I think you should just give up on loading the AMD hardware decoder. There's likely bigger issues with the user's machine!

you assume wrong :)

The code is correct, we want ProgramW6432, not CSIDL_PROGRAM_FILES

> Why are you getting the ProgramFiles path from Windows, and then grabbing the drive, and then reconstructing a new path? Why can't you trust the value returned by Windows?

because there are no reliable way of getting the value out of windows. One that wouldn't involved heavily modifying the sandbox or exporting content there.

Here is the Chromium code to retrieve that value, those functions are available in the sandbox:
https://cs.chromium.org/chromium/src/base/base_paths_win.cc?type=cs&q=DIR_PROGRAM_FILES6432&l=69

You would need all the base::win::OSInfo utilities, being able to retrieve the wow64_status and so forth.
It would be 1000s of line of code.

We discussed this issue, and my simpler approach to it during our last stand up meeting :(

> It doesn't look like you're constructing a valid path here. It will be something like "A3:\\Program Files", which is not a valid Windows path format.
> 
> Why can't you just return the value of ShGetKnownFolderPath?

no 'A' + 2 is 'C', and 'C' - 'A' is 2.
'A' is a character, not a string!
Comment on attachment 8897348 [details]
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.

https://reviewboard.mozilla.org/r/168658/#review174242

> We have initialization spelt with an 's' and a 'z' in the same sentance here.

was in the original code. but sure.
Comment on attachment 8897348 [details]
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.

https://reviewboard.mozilla.org/r/168658/#review174744

::: dom/media/platforms/wmf/WMFUtils.cpp:156
(Diff revisions 1 - 2)
> -GetProgramFilePath()
> +GetProgramW6432Path()
>  {
>    nsString path;
> -  wchar_t programPath[MAX_PATH + 1];
> +  PWSTR programPath = nullptr;
>  
> -  if (FAILED(SHGetFolderPath(NULL, CSIDL_PROGRAM_FILES, NULL,
> +  if (FAILED(SHGetKnownFolderPath(FOLDERID_ProgramFiles, 0, 0, &programPath))) {

Could you remind me why you couldn't rely on the environment variables set in WoW64?

I did a quick check and PR_GetEnv("ProgramW6432") returns "C:\Program Files" in both 64bit and 32bit Firefox on a 64bit Windows 10 OS.

In 32bit Firefox on 32bit Windows 7 OS PR_GetEnv("ProgramW6432") returns null, but PR_GetEnv("ProgramFiles") returns "C:\Program Files".

This testing was performed with e10s and sandboxing enabled.

Can you use PR_GetEnv("ProgramW6432") if it's non-null, and otherwise use PR_GetEnv("ProgramFiles")?
Attachment #8897348 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #9)
> Could you remind me why you couldn't rely on the environment variables set
> in WoW64?

Maybe because you thought we still had to support Windows XP and Vista? We do not support them anymore, so we should be able to use these env vars now, right?
Comment on attachment 8897348 [details]
Bug 1332234 - P1. Add support for AMD's VP9 hardware decoder.

https://reviewboard.mozilla.org/r/168658/#review175222
Attachment #8897348 - Flags: review?(cpearce) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/057c3caace33
P1. Add support for AMD's VP9 hardware decoder. r=cpearce
Blocks: 1394589
Blocks: 1394590
https://hg.mozilla.org/mozilla-central/rev/057c3caace33
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1417241
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: