1281259 - Port DXVA to gfxConfig

Status: NEW

(Core :: Graphics, defect, P3)

Comment 1

[Ernest Yim [:ernest]]

Attachment: Bug 1281259 - Port DXVA to gfxConfig

Review commit: https://reviewboard.mozilla.org/r/60112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60112/

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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 3

[Ernest Yim [:ernest]]

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/

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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 5

[Ernest Yim [:ernest]]

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/

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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 7

[Ernest Yim [:ernest]]

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/

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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)

Comment 9

[Ernest Yim [:ernest]]

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 10

[Ernest Yim [:ernest]]

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/

Comment 11

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 12

[Ernest Yim [:ernest]]

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/

Comment 13

[Ernest Yim [:ernest]]

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 14

[Ernest Yim [:ernest]]

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 15

[Ernest Yim [:ernest]]

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 16

[Ernest Yim [:ernest]]

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 17

[Ernest Yim [:ernest]]

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 18

[Ernest Yim [:ernest]]

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 19

[Ernest Yim [:ernest]]

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 20

[Ernest Yim [:ernest]]

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 21

[Ernest Yim [:ernest]]

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 22

[Ernest Yim [:ernest]]

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 23

[Ernest Yim [:ernest]]

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 24

[Ernest Yim [:ernest]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23eb41b8995e

Comment 25

[Ernest Yim [:ernest]]

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/

Comment 26

[Ernest Yim [:ernest]]

re-tested GTest and passes https://treeherder.mozilla.org/#/jobs?repo=try&revision=e40b412b3cf4, should be ready to land now

Comment 27

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 28

[Ernest Yim [:ernest]]

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 29

[Ernest Yim [:ernest]]

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/

Comment 30

[Jeff Muizelaar [:jrmuizel]]

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

Comment 31

[Carsten Book [:Tomcat]]

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!

Comment 32

[Ernest Yim [:ernest]]

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/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6d147ba37b5
Port DXVA to gfxConfig r=jrmuizel

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/autoland/rev/9c84392108d9 for apparently making Mn-e10s permafail on the Windows VM:
https://treeherder.mozilla.org/logviewer.html#?job_id=798741&repo=autoland

Looks like a known intermittent, but https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=cc98fa9758625dad454895ee1206451464ca1a58&bugfiler&filter-searchStr=windows%20vm%20mn-e%20debug&selectedJob=798741&tochange=9c84392108d91ea0a598b13e0acfd6b086e2952a

Comment 35

[Ernest Yim [:ernest]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef597ed8c093

Comment 36

[Ernest Yim [:ernest]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73894e8c0cc8

Comment 37

[Ernest Yim [:ernest]]

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-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.

Comment 38

[Benoit Girard (:BenWa)]

Attachment: Bug 1281259 - Port DXVA configuration to gfxConfig. r?

Review commit: https://reviewboard.mozilla.org/r/72674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72674/

Comment 39

[Benoit Girard (:BenWa)]

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.