1280743 - dom/media/gmp/GMPParent.cpp fails to compile with --disable-eme

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

dom/media/gmp/GMPParent.cpp:43:10: fatal error: 'mozilla/dom/WidevineCDMManifestBinding.h' file not found
#include "mozilla/dom/WidevineCDMManifestBinding.h"
         ^

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix?

This works for me, but I don't know this code at all...

Comment 2

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

Comment on attachment 8763317 [details] [diff] [review]
fix?

Review of attachment 8763317 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/gmp/GMPParent.cpp
@@ +819,5 @@
>    manifestFile->AppendRelativePath(NS_LITERAL_STRING("manifest.json"));
>    return ReadChromiumManifestFile(manifestFile);
> +#endif
> +
> +  return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);

Fly-by comment: When MOZ_EME is #defined, this last 'return' will be dead code after the previous 'return'! I would suggest using #else...#endif around it instead.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24999edead4e
Put Widevine stuff under #ifdef MOZ_EME to make this file compile under --disable-eme.  r=cpearce

Comment 4

[Mats Palmgren (inactive)]

Good catch Gerald, I pushed with the #else as suggested.

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/24999edead4e

Comment 6

[Maire Reavy [:mreavy]]

Chris -- Is Fx 49 affected?  If so, does it make sense to uplift this to Fx 49?

Comment 7

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

(In reply to Maire Reavy (On PTO July 23rd to 31st) from comment #6)
> Chris -- Is Fx 49 affected?  If so, does it make sense to uplift this to Fx
> 49?

Gerald: Can you uplift this if the build is broken without it please?

Comment 8

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

49 is not affected, as the Widevine code there hides behind '#ifdef MOZ_WIDEVINE_EME', including the #include line that caused problem in 50.

Comment 10

[Masatoshi Kimura [:emk]]

Actually the include guard has been removed:
https://hg.mozilla.org/releases/mozilla-beta/diff/7b8879ab0e02/dom/media/gmp/GMPParent.cpp

Comment 11

[kevink9876543]

I had put up a patch for beta 49 in bug 1296627 (not fully tested yet).  Is it useful here?

Comment 12

[kevink9876543]

(In reply to kevink9876543 from comment #11)
> I had put up a patch for beta 49 in bug 1296627 (not fully tested yet).  Is
> it useful here?

Tested my local beta 49 builds w/ attachment 8783123 [details] [diff] [review] and all is working as expected.  I'm confused though because the patch in this bug is much smaller.  What effect are the "extra" parts in my patch have, if any?  Is it Gecko-49-specific?

Comment 13

[Mats Palmgren (inactive)]

Gerald, can you check v49 again and uplift if necessary, thanks!

Comment 14

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

Comment on attachment 8763317 [details] [diff] [review]
fix?

Approval Request Comment
[Feature/regressing bug #]: --disable-eme
[User impact if declined]: Not possible to build Firefox when EME is not wanted
[Describe test coverage new/current, TreeHerder]: In 50 for 2 months
[Risks and why]: No risk that I can see, as this is just disabling the code dealing with Widevine
[String/UUID change made/needed]: None

Note that this was initially (end July) not needed in 49, but then bug 1276132 uplifted some EME code to 49 on August 10, including the part that was failing to build with --disable-eme.

Comment 15

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

(In reply to kevink9876543 from comment #12)
> I'm confused though because the
> patch in this bug [compared to bug 1296627] is much smaller.  What effect are the "extra" parts in my
> patch have, if any?

The patch in bug 1296627 also #ifdef'd-out the full Widevine-related function declarations and definitions, instead of just not calling them.
I'm choosing the patch in this bug here for uplift, because it is touching less code, and it has been in the codebase for 2 months already so it carries lower risk.

Comment 16

[Gerry Chang [:gchang]]

Hi :gerald,
Since there is no feature/regression bug associated with this bug, if this is not in beta, will it be a critical issue?

Comment 17

[Mats Palmgren (inactive)]

I suspect there are some downstream consumers (the Tor browser?) that use
--disable-eme.  Not breaking them might be a reason to uplift.

Comment 18

[kevink9876543]

(In reply to Gerry Chang [:gchang] from comment #16)
> Since there is no feature/regression bug associated with this bug,
Maybe it wasn't quite made clear.  There *is* a regressing bug here, that would be bug 1276132.

Comment 19

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

Thank you kevink, and :emk (comment 10). There is indeed a regression from bug 1276132 when it was uplifted to beta on August 10 (as I implied in my uplift request in comment 14).

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8763317 [details] [diff] [review]
fix?

We want to be able to disable this for non-eme builds, please uplift to beta.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/16dac3dd9af0