Closed
Bug 1271169
Opened 6 years ago
Closed 6 years ago
[EME] Unit test for NodeId generation
Categories
(Core :: Audio/Video: GMP, defect, P2)
Core
Audio/Video: GMP
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(3 files)
We should have a unit test to ensure that node Id generation doesn't regress. This broke Netflix in Bug 1264497.
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d64783956f
Updated•6 years ago
|
Rank: 25
Priority: -- → P2
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58ec1a82be43
Assignee | ||
Comment 3•6 years ago
|
||
I want the EME device binding/nodeId code to be callable from gtests, as well as from in plugin-container. First step is to move the device binding code into a discrete file, so I can also link that into gtests, and call it from there to compare the result with what's in the GMP process. Review commit: https://reviewboard.mozilla.org/r/54116/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54116/
Attachment #8755227 -
Flags: review?(gsquelart)
Attachment #8755228 -
Flags: review?(mh+mozilla)
Attachment #8755229 -
Flags: review?(gsquelart)
Assignee | ||
Comment 4•6 years ago
|
||
I want the EME device binding/nodeId code to be callable from gtests, as well as in plugin-container. I need this because I want to add a gtest that ensures that we don't regress the EME/GMP device binding code. I want to call the GMP device binding code in the gtest and in the GMP process, and compare the result. So we need to make it possible to link the device binding code into the gtests as well as plugin-container. So move all code that device binding calls into librlz, to make it easier to link against all the code required. Unfortunately the gtests are linked against a dynamic MSVCRT, but librlz, where the device binding code lives, is linked with static MSVCRT. So in order to link into gtests, we must remove the force static runtime directive from librlz's moz.build. Note: the device binding code needs to be statically linked into plugin-container so that it's covered by the Adobe CDM's voucher tool. Review commit: https://reviewboard.mozilla.org/r/54118/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54118/
Assignee | ||
Comment 5•6 years ago
|
||
Call the device binding code in gtests, and have a test GMP which returns the device ID it was passed in a message, so that we can verify that the id the GMP was passed is the same as the id generated in the parent. Review commit: https://reviewboard.mozilla.org/r/54120/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54120/
Comment on attachment 8755227 [details] MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald https://reviewboard.mozilla.org/r/54116/#review51134
Attachment #8755227 -
Flags: review?(gsquelart) → review+
Attachment #8755229 -
Flags: review?(gsquelart) → review+
Comment on attachment 8755229 [details] MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald https://reviewboard.mozilla.org/r/54120/#review51140 r+ for your changes. Noticed this nearby, if you want to improve it: ::: dom/media/gtest/TestGMPCrossOrigin.cpp:639 (Diff revision 1) > Updates(GMPStorageTest* aRunner, nsTArray<nsCString>&& aUpdates) > : mRunner(aRunner), > mUpdates(aUpdates) The last initializer should probably be 'mUpdates(Move(aUpdates))'
Comment 8•6 years ago
|
||
Comment on attachment 8755228 [details] MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium https://reviewboard.mozilla.org/r/54118/#review51332 Nothing to say about the patch, but I have something to say about the commit message: > Unfortunately the gtests are linked against a dynamic MSVCRT, but librlz, where > the device binding code lives, is linked with static MSVCRT. So in order to > link into gtests, we must remove the force static runtime directive from > librlz's moz.build. That's actually not true since bug 1035125, which removed the USE_STATIC_LIBS = True in rlz/moz.build. The FORCE_STATIC_LIB you're removing in this patch is actually a no-op because it's the default when you have a Library(), and is not related to the static CRT.
Attachment #8755228 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8) > That's actually not true since bug 1035125, which removed the > USE_STATIC_LIBS = True in rlz/moz.build. The FORCE_STATIC_LIB you're > removing in this patch is actually a no-op because it's the default when you > have a Library(), and is not related to the static CRT. Ha! I was sure the behaviour changed around the 18th, so that would explain why! Thanks.
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8755227 [details] MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54116/diff/1-2/
Attachment #8755227 -
Attachment description: MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r?gerald → MozReview Request: Bug 1271169 - Move EME/GMP device binding code into GMPDeviceBinding.h/cpp. r=gerald
Attachment #8755228 -
Attachment description: MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r?glandium → MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium
Attachment #8755229 -
Attachment description: MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r?gerald → MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8755228 [details] MozReview Request: Bug 1271169 - Move all device binding code into librlz and remove static runtime requirement. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54118/diff/1-2/
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 8755229 [details] MozReview Request: Bug 1271169 - Add gtest to ensure EME device binding works. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54120/diff/1-2/
Comment 13•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/200726fdfb53 https://hg.mozilla.org/integration/mozilla-inbound/rev/4da6cece7b58 https://hg.mozilla.org/integration/mozilla-inbound/rev/57fa89168a44
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/200726fdfb53 https://hg.mozilla.org/mozilla-central/rev/4da6cece7b58 https://hg.mozilla.org/mozilla-central/rev/57fa89168a44
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•