Open Bug 1281259 Opened 5 years ago Updated 4 years ago

Port DXVA to gfxConfig

Categories

(Core :: Graphics, defect, P3)

Unspecified
Windows
defect

Tracking

()

People

(Reporter: ernest, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

No description provided.
https://reviewboard.mozilla.org/r/60112/#review56990

::: gfx/thebes/gfxPlatform.cpp:2114
(Diff revision 1)
>      Preferences::GetBool("media.windows-media-foundation.use-dxva", true) &&
>  #endif
>        NS_SUCCEEDED(gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_HARDWARE_VIDEO_DECODING,
>                                                 discardFailureId, &status))) {
> -      if (status == nsIGfxInfo::FEATURE_STATUS_OK || gfxPrefs::HardwareVideoDecodingForceEnabled()) {
> +    if (status == nsIGfxInfo::FEATURE_STATUS_OK || gfxPrefs::HardwareVideoDecodingForceEnabled()) {
> -         sLayersSupportsHardwareVideoDecoding = true;
> +      hwVideoDecFeature.EnableByDefault();

This should probably move out of the if condition and then adjust the feature status according to the results of gfxInfo and the pref. see UserForceEnable()
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/1-2/
https://reviewboard.mozilla.org/r/60112/#review57106

::: gfx/thebes/gfxPlatform.cpp:2109
(Diff revision 2)
>  
> -  if (Preferences::GetBool("media.hardware-video-decoding.enabled", false) &&
> +  FeatureState& hwVideoDecFeature = gfxConfig::GetFeature(Feature::HW_VIDEO_DECODING);
> +
> +  // force enabled feature
> +  if (gfxPrefs::HardwareVideoDecodingForceEnabled()) {
> +    hwVideoDecFeature.UserForceEnable("User force-enabled video decoding.");

Defaults need to be set first. Check the force enabled pref after setting the default.
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/2-3/
https://reviewboard.mozilla.org/r/60112/#review57292

::: gfx/thebes/gfxPlatform.cpp
(Diff revisions 2 - 3)
>    nsCString failureId;
>    if (!IsGfxInfoStatusOkay(nsIGfxInfo::FEATURE_HARDWARE_VIDEO_DECODING, &message, failureId)) {
>      hwVideoDecFeature.Disable(FeatureStatus::Blacklisted, message.get(), failureId);
>    }
>  
> -  Preferences::AddBoolVarCache(&sLayersHardwareVideoDecodingFailed,

It looks like you should still be using this pref to adjust gfx::Feature::HW_VIDEO_DECODING

::: gfx/thebes/gfxPlatform.cpp:2172
(Diff revisions 2 - 3)
>                           NS_LITERAL_CSTRING("FEATURE_FAILURE_COMP_SAFEMODE"));
>    }
>  }
>  
>  bool
>  gfxPlatform::CanUseHardwareVideoDecoding()

This function needs to go away.
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/3-4/
Attachment #8764064 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

https://reviewboard.mozilla.org/r/60112/#review57714

::: gfx/thebes/gfxPlatform.h:179
(Diff revision 4)
>  
>      /**
> +     * Used to update (enable or disable) hw video decode feature based on pref
> +     * Dummy parameters are used to work with existing RegisterCallback func
> +     */
> +    static void UpdateHWDecBasedOnPref(const char* aPref, void* aClosure);

No need to have this as a class member. Just use a static function in gfxPlatform.cpp

::: gfx/thebes/gfxWindowsPlatform.cpp:405
(Diff revision 4)
>    UpdateANGLEConfig();
>    UpdateRenderMode();
>  }
>  
>  bool
>  gfxWindowsPlatform::CanUseHardwareVideoDecoding()

These checks will need to be integrated into the HW_VIDEO_DECODING feature because this function is no longer being called. (I'm actually surprised that this compiled because as far as I can see CanUseHardwareVideoDecoding is marked override in gfxWindowsPlatform.h but no longer overrides anything after your patch)
https://reviewboard.mozilla.org/r/60112/#review57292

> It looks like you should still be using this pref to adjust gfx::Feature::HW_VIDEO_DECODING

not the cleanest implementation, but the default RegisterCallback expects a void (*)(const char *, void *)'), so I threw in unused parameters into the callback function.
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/4-5/
Attachment #8764064 - Flags: review- → review?(jmuizelaar)
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

https://reviewboard.mozilla.org/r/60112/#review57720

::: gfx/thebes/gfxPlatform.cpp:2139
(Diff revisions 4 - 5)
>    // force enabled feature
>    if (gfxPrefs::HardwareVideoDecodingForceEnabled()) {
>      hwVideoDecFeature.UserForceEnable("User force-enabled video decoding.");
>    }
>  
> +#ifdef XP_WIN

This should probably go in gfxWindowPlatform instead.
Attachment #8764064 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/5-6/
Attachment #8764064 - Flags: review- → review?(jmuizelaar)
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/6-7/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/7-8/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/8-9/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/9-10/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/10-11/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/11-12/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/12-13/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/13-14/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/14-15/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/15-16/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/16-17/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/17-18/
re-tested GTest and passes https://treeherder.mozilla.org/#/jobs?repo=try&revision=e40b412b3cf4, should be ready to land now
Flags: needinfo?(jmuizelaar)
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

https://reviewboard.mozilla.org/r/60112/#review61122

::: gfx/thebes/gfxPlatform.h:177
(Diff revision 18)
>      static void Shutdown();
>  
>      static void InitLayersIPC();
>      static void ShutdownLayersIPC();
>  
> +    virtual void InitHWVideoDecoodingConfig(mozilla::gfx::FeatureState& hwVideoDecFeature){};

Decoding instead of Decooding

::: gfx/thebes/gfxPlatform.cpp:2140
(Diff revision 18)
> -                               "media.hardware-video-decoding.failed",
> -                               false);
> +  if (gfxPrefs::HardwareVideoDecodingForceEnabled()) {
> +    hwVideoDecFeature.UserForceEnable("User force-enabled video decoding.");
> +  }
> +
> +#ifdef XP_WIN
> +  gPlatform->InitHWVideoDecoodingConfig(hwVideoDecFeature);

Let's just call this unconditionally. That makes things less surprising if anyone implements it on other platforms.

::: gfx/thebes/gfxWindowsPlatform.h:282
(Diff revision 18)
>      int8_t mUseClearTypeForDownloadableFonts;
>      int8_t mUseClearTypeAlways;
>  
>  private:
>      void Init();
> +

You can drop this change.
Attachment #8764064 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/18-19/
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/19-20/
Keywords: checkin-needed
has problems to apply:

Hunk #2 FAILED at 168
1 out of 3 hunks FAILED -- saving rejects to file gfx/thebes/gfxPlatform.h.rej
patch failed to apply


can you take a look, thanks!
Flags: needinfo?(eyim)
Keywords: checkin-needed
Comment on attachment 8764064 [details]
Bug 1281259 - Port DXVA to gfxConfig

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60112/diff/20-21/
Flags: needinfo?(eyim)
http://archive.mozilla.org/pub/firefox/try-builds/eyim@mozilla.com-1f04741a99d77ae3633b41c0fc3a8c97029e00ad/try-win32-debug/try_win7_vm-debug_test-marionette-e10s-bm139-tests1-windows-build172.txt.gz

10:28:59     INFO -  TEST-START | test_refresh_firefox.py TestFirefoxRefresh.testReset
10:31:01     INFO -  Failed to gather test failure debug.
10:31:01     INFO -  Traceback (most recent call last):
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette\runner\base.py", line 550, in gather_debug
10:31:01     INFO -      with marionette.using_context(marionette.CONTEXT_CHROME):
10:31:01     INFO -    File "c:\mozilla-build\python27\Lib\contextlib.py", line 17, in __enter__
10:31:01     INFO -      return self.gen.next()
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 1319, in using_context
10:31:01     INFO -      scope = self._send_message("getContext", key="value")
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\decorators.py", line 48, in _
10:31:01     INFO -      m.force_shutdown()
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\decorators.py", line 42, in _
10:31:01     INFO -      return func(*args, **kwargs)
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 682, in _send_message
10:31:01     INFO -      msg = self.client.request(name, params)
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\transport.py", line 273, in request
10:31:01     INFO -      self.send(cmd)
10:31:01     INFO -    File "c:\slave\test\build\venv\lib\site-packages\marionette_driver\transport.py", line 247, in send
10:31:01     INFO -      sent = self.sock.send(payload[totalsent:])
10:31:01     INFO -    File "c:\mozilla-build\python27\Lib\socket.py", line 170, in _dummy
10:31:01     INFO -      raise error(EBADF, 'Bad file descriptor')
10:31:01     INFO -  IOError: Process has been closed (Exit code: 0) (Reason: [Errno 9] Bad file descriptor)
10:31:01    ERROR -  TEST-UNEXPECTED-ERROR | test_refresh_firefox.py TestFirefoxRefresh.testReset | IOError: Process has been closed (Exit code: 0) (Reason: Timed out waiting for port 2828!)

Just to update, there is a timeout when running "test_refresh_firefox.py TestFirefoxRefresh.testReset" in marionette test suite. Not really sure what my patch has to do with this at all, especially only for windows 7 debug only. Seems like it may be some timing issue.
Blocks: 1294721
Flags: needinfo?(bgirard)
Rebased the patch. Found a typo that was causing an override to be missed. Let's push to try again to see if that was the issue.
Flags: needinfo?(jmuizelaar)
Assignee: ernest-yim → nobody
Flags: needinfo?(bgirard)
Priority: -- → P3
Whiteboard: [gfx-noted]
You need to log in before you can comment on or make changes to this bug.