1332530 - Remove GMP device binding

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We can remove the device binding code now that we're dropping Adobe EME, and move the GMP loading code back into xul.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Remove librlz and EME/GMP device binding code.

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

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Move GMPLoader code out of plugin-container and back into XUL.

The GMPLoader code was in plugin-container so that it was covered by
Adobe's voucher of plugin-container, but that's no longer necessary.

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

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Flatten GMPLoader and GMPLoaderImpl.

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

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined.

GMP gtests fail on ASAN builds now since the GMPLoader requires a sandbox
starter, and ASAN doesn't run with a GMP sandbox. So only enforce that we
need a sandbox starter if we've built with sandboxing enabled.

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Remove code to pass nodeId to GMP process.

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

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1332530 - Remove GMP enum storage names.

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

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8831013 [details]
Bug 1332530 - Remove librlz and EME/GMP device binding code.

https://reviewboard.mozilla.org/r/107658/#review108824

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8831014 [details]
Bug 1332530 - Move GMPLoader code out of plugin-container and back into XUL.

https://reviewboard.mozilla.org/r/107660/#review108830

r+ with style nit:

::: dom/media/gmp/GMPLoader.cpp:207
(Diff revision 1)
>    }
>  }
>  #endif
> +
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +class WinSandboxStarter : public mozilla::gmp::SandboxStarter {

I see you've moved this code from other files, but this file uses a different (and more correct!) style with class/function braces '{' on their own line, so could you please make the imported code consistent with its new home?

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8831015 [details]
Bug 1332530 - Flatten GMPLoader and GMPLoaderImpl.

https://reviewboard.mozilla.org/r/107662/#review108832

r+ with style nits:

::: dom/media/gmp/GMPLoader.h:57
(Diff revision 1)
> -//
>  // Load() takes an optional GMPAdapter which can be used to adapt non-GMPs
>  // to adhere to the GMP API.
>  class GMPLoader {
>  public:
> -  virtual ~GMPLoader() {}
> +  explicit GMPLoader();

'explicit' is meaningless with a no-argument constructor. But it's harmless, so it's fine to keep if you prefer having it.

::: dom/media/gmp/GMPLoader.cpp:243
(Diff revision 1)
> -UniquePtr<GMPLoader> CreateGMPLoader() {
> -  return MakeUnique<GMPLoaderImpl>(MakeSandboxStarter());
> +GMPLoader::GMPLoader()
> +{
> +  mSandboxStarter = MakeSandboxStarter();

Could you please move this initialization into the constructor's member initializer list?

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8831017 [details]
Bug 1332530 - Remove code to pass nodeId to GMP process.

https://reviewboard.mozilla.org/r/107666/#review108836

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8831018 [details]
Bug 1332530 - Remove GMP enum storage names.

https://reviewboard.mozilla.org/r/107668/#review108838

r+ as it does what you've said in the short commit description...
But would you mind adding why you're removing this seemingly-useful API? (Is it just actually not that useful?)

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8831016 [details]
Bug 1332530 - Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined.

https://reviewboard.mozilla.org/r/107664/#review108916

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Gerald Squelart [:gerald] from comment #12)
> Comment on attachment 8831018 [details]
> Bug 1332530 - Remove GMP enum storage names.
> 
> https://reviewboard.mozilla.org/r/107668/#review108838
> 
> r+ as it does what you've said in the short commit description...
> But would you mind adding why you're removing this seemingly-useful API? (Is
> it just actually not that useful?)

There is no analogous API in the Chromium CDM API. So since we've removed the Adobe Primetime CDM, and move ClearKey to the Chromium API, there's no callers for this API. So no point keeping it around.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831013 [details]
Bug 1332530 - Remove librlz and EME/GMP device binding code.

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

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831014 [details]
Bug 1332530 - Move GMPLoader code out of plugin-container and back into XUL.

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

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831015 [details]
Bug 1332530 - Flatten GMPLoader and GMPLoaderImpl.

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

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831016 [details]
Bug 1332530 - Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined.

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

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831017 [details]
Bug 1332530 - Remove code to pass nodeId to GMP process.

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

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8831018 [details]
Bug 1332530 - Remove GMP enum storage names.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14c5ba1485bd
Remove librlz and EME/GMP device binding code. r=gerald
https://hg.mozilla.org/integration/autoland/rev/57c613d80dc3
Move GMPLoader code out of plugin-container and back into XUL. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6b3ca23424dc
Flatten GMPLoader and GMPLoaderImpl. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6a31f295cd81
Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined. r=jld
https://hg.mozilla.org/integration/autoland/rev/b599c9fa4080
Remove code to pass nodeId to GMP process. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8977e2dfe179
Remove GMP enum storage names. r=gerald

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14c5ba1485bd
https://hg.mozilla.org/mozilla-central/rev/57c613d80dc3
https://hg.mozilla.org/mozilla-central/rev/6b3ca23424dc
https://hg.mozilla.org/mozilla-central/rev/6a31f295cd81
https://hg.mozilla.org/mozilla-central/rev/b599c9fa4080
https://hg.mozilla.org/mozilla-central/rev/8977e2dfe179