Closed Bug 1520711 Opened 5 years ago Closed 5 years ago

EME key system constants are used with UTF-8 functions where ASCII functions would do

Categories

(Core :: Audio/Video, enhancement, P5)

Product:
Core
Component:
Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: hsivonen, Assigned: f20160385)

Details

Attachments

(1 file)

bug 1520711 - EME key system constants are used with UTF-8 functions where ASCII functions would do
47 bytes, text/x-phabricator-request
Details | Review

kEMEKeySystemClearkey and kEMEKeySystemWidevine are ASCII but are used with comparison and conversion functions that handle full UTF-8. It would be slightly more efficient (not that it matters in practice) to a) declare them as POD literals instead of NS_NAMED_LITERAL_CSTRING and b) when comparing with or assigning to UTF-16, use the nsAString methods that deal with ASCII / ASCII literals.

Hi, I want to work on this bug. Could you please guide me a little on where I could find this file which is needed to be changed?

Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #1)

Hi, I want to work on this bug. Could you please guide me a little on where I could find this file which is needed to be changed?

You can search for the constants mentioned in comment 0 using searchfox:
https://searchfox.org/mozilla-central/search?q=kEMEKeySystemClearkey&path=
https://searchfox.org/mozilla-central/search?q=kEMEKeySystemWidevine&path=

If they were #defined as plain string literals, they could be used with
https://searchfox.org/mozilla-central/source/xpcom/string/nsTStringRepr.h#241

Flags: needinfo?(hsivonen)

So, should they be defined like this: ?

const char* kEMEKeySystemWidevine = "com.widevine.alpha";
Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #3)

So, should they be defined like this: ?

const char* kEMEKeySystemWidevine = "com.widevine.alpha";

I expect that to lose the string length and not to work with EqualsLiteral. Perhaps auto instead of char* works (I haven't tested). If it doesn't work with EqualsLiteral, then #define EME_KEY_SYSTEM_WIDEVINE "com.widevine.alpha", which should work for sure.

Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #4)

I expect that to lose the string length and not to work with EqualsLiteral. Perhaps auto instead of char* works (I haven't tested). If it doesn't work with EqualsLiteral, then #define EME_KEY_SYSTEM_WIDEVINE "com.widevine.alpha", which should work for sure.
I'm getting an error in build by using both the ways. The error reason is mainly:
https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.h#48

How should this be changed?

Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #5)

(In reply to Henri Sivonen (:hsivonen) from comment #4)

I expect that to lose the string length and not to work with EqualsLiteral. Perhaps auto instead of char* works (I haven't tested). If it doesn't work with EqualsLiteral, then #define EME_KEY_SYSTEM_WIDEVINE "com.widevine.alpha", which should work for sure.
I'm getting an error in build by using both the ways. The error reason is mainly:
https://searchfox.org/mozilla-central/source/dom/media/VideoUtils.h#48

How should this be changed?

These externs should go away and instead this file should include the header that the new #defines or constants are in.

Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #6)

These externs should go away and instead this file should include the header that the new #defines or constants are in.

On doing this, I'm getting compilation error at 3 points:
https://searchfox.org/mozilla-central/source/dom/media/eme/EMEUtils.cpp#51
https://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeySystemAccess.cpp#247
https://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeySystemAccess.cpp#249

How do I correct these?

Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #7)

(In reply to Henri Sivonen (:hsivonen) from comment #6)

These externs should go away and instead this file should include the header that the new #defines or constants are in.

On doing this, I'm getting compilation error at 3 points:
https://searchfox.org/mozilla-central/source/dom/media/eme/EMEUtils.cpp#51
https://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeySystemAccess.cpp#247
https://searchfox.org/mozilla-central/source/dom/media/eme/MediaKeySystemAccess.cpp#249

How do I correct these?

If you have #define EME_KEY_SYSTEM_CLEARKEY "org.w3.clearkey", it should be possible to do aKeySystem.EqualsLiteral(EME_KEY_SYSTEM_CLEARKEY) and clearkey.mKeySystem.AssignLiteral(EME_KEY_SYSTEM_CLEARKEY). It should also be possible to change the argument type of HavePluginForKeySystem to const char*.

Flags: needinfo?(hsivonen)

EME key system constants are used with UTF-8 functions where ASCII functions would do

What changes do you suggest in these: ?
https://searchfox.org/mozilla-central/source/dom/media/gmp/ChromiumCDMAdapter.cpp#122-123

Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #10)

What changes do you suggest in these: ?
https://searchfox.org/mozilla-central/source/dom/media/gmp/ChromiumCDMAdapter.cpp#122-123

EME_KEY_SYSTEM_WIDEVINE and mozilla::ArrayLength(EME_KEY_SYSTEM_WIDEVINE) - 1

Flags: needinfo?(hsivonen)

After making all the suitable changes, my build is still failing at these points:

4:30.25 In file included from /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dom/media/eme/Unified_cpp_dom_media_eme0.cpp:74:
 4:30.25 /home/khyati/src/mozilla-unified/dom/media/eme/MediaKeySystemAccess.cpp:98:21: error: no matching function for call to 'HaveGMPFor'
 4:30.25   bool havePlugin = HaveGMPFor(api, {aKeySystem});
 4:30.25                     ^~~~~~~~~~
 4:30.25 /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/GMPUtils.h:71:6: note: candidate function not viable: cannot convert initializer list argument to 'nsTArray<nsCString>' (aka 'nsTArray<nsTString<char> >')
 4:30.25 bool HaveGMPFor(const nsCString& aAPI, nsTArray<nsCString>&& aTags);
 4:30.25      ^

4:30.25 In file included from /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dom/media/eme/Unified_cpp_dom_media_eme0.cpp:74:
 4:30.25 /home/khyati/src/mozilla-unified/dom/media/eme/MediaKeySystemAccess.cpp:110:8: error: no matching function for call to 'HavePluginForKeySystem'
 4:30.25   if (!HavePluginForKeySystem(NS_ConvertUTF16toUTF8(aKeySystem))) {
 4:30.25        ^~~~~~~~~~~~~~~~~~~~~~
 4:30.25 /home/khyati/src/mozilla-unified/dom/media/eme/MediaKeySystemAccess.cpp:95:13: note: candidate function not viable: no known conversion from 'NS_ConvertUTF16toUTF8' to 'const nsAString' (aka 'const nsTSubstring<char16_t>') for 1st argument
 4:30.25 static bool HavePluginForKeySystem(const nsAString& aKeySystem) {
 4:30.26             ^

4:30.27 /home/khyati/src/mozilla-unified/dom/media/eme/MediaKeySystemAccess.cpp:251:9: error: no matching function for call to 'HavePluginForKeySystem'
 4:30.27     if (HavePluginForKeySystem(EME_KEY_SYSTEM_CLEARKEY)) {
 4:30.27         ^~~~~~~~~~~~~~~~~~~~~~
 4:30.27 /home/khyati/src/mozilla-unified/dom/media/eme/MediaKeySystemAccess.cpp:95:13: note: candidate function not viable: no known conversion from 'const char [16]' to 'const nsAString' (aka 'const nsTSubstring<char16_t>') for 1st argument
 4:30.27 static bool HavePluginForKeySystem(const nsAString& aKeySystem) {
 4:30.27             ^
Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #12)

After making all the suitable changes, my build is still failing at these points:

Sorry, it's best to keep the signature of HavePluginForKeySystem unchanged and on call sites, and when calling it with a constant, doing

nsCString keySystem;
keySystem.AssignLiteral(EME_KEY_SYSTEM_CLEARKEY);

And then passing keySystem to HavePluginForKeySystem.

(Likewise for Widevine.)

Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #13)

(In reply to Khyati Agarwal from comment #12)

After making all the suitable changes, my build is still failing at these points:

Sorry, it's best to keep the signature of HavePluginForKeySystem unchanged and on call sites, and when calling it with a constant, doing

nsCString keySystem;
keySystem.AssignLiteral(EME_KEY_SYSTEM_CLEARKEY);

And then passing keySystem to HavePluginForKeySystem.

(Likewise for Widevine.)

It worked. Thanks a lot for the clarification. Please check the updated patch. It built successfully on my machine.

Flags: needinfo?(hsivonen)
Flags: needinfo?(hsivonen)

Please assign this bug to me.

Flags: needinfo?(hsivonen)

(In reply to Khyati Agarwal from comment #15)

Please assign this bug to me.

Assigned. Thank you!

Let's see a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34020656c3fcf40faa83d1726eeae69600db7541

Assignee: nobody → f20160385
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)

The try run failed with what look like harness issues. Let's try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b290dfa3445e9155cc9b34957af1c44d15405f

Ignore the previous one.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16101bdfd7f13a8071e9f8011b89cde3d49c6d34
What do you think about this? Is there something we can do to improve it?

Flags: needinfo?(hsivonen)

All are known intermittents. The only failure relevant to this change is https://treeherder.mozilla.org/#/jobs?repo=try&revision=16101bdfd7f13a8071e9f8011b89cde3d49c6d34&selectedJob=238719924 . I retriggered it to see if it's still intermittent.

Flags: needinfo?(hsivonen)

(In reply to Henri Sivonen (:hsivonen) from comment #20)

All are known intermittents. The only failure relevant to this change is https://treeherder.mozilla.org/#/jobs?repo=try&revision=16101bdfd7f13a8071e9f8011b89cde3d49c6d34&selectedJob=238719924 . I retriggered it to see if it's still intermittent.

This was intermittent. This is now ready for landing, so you can add the checkin-needed keyword.

Thanks :)

Keywords: good-first-bug → checkin-needed

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/517e71d9bccc
EME key system constants are used with UTF-8 functions where ASCII functions would do r=hsivonen,jya

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/517e71d9bccc

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Thanks for fixing this!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: