Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1381463003:
Add warning message for empty robustness. (Closed)
|
Created:
5 years, 2 months ago by xhwang Modified:
5 years, 2 months ago CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, philipj_slow, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd warning message for empty robustness.
Add a warning message when
a. the key system is widevine, and
b. any video capability passed into requestMediaKeySystemAccess() has empty robustness level.
This warning message will be removed after issue 482277 is fixed. See the bug
for more details.
Also add UMA to report the empty robustness level.
BUG=482277
TEST=Tested with clearkey and widevine.
R=ddorwin@chromium.org, isherman@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/0f9748c2c6fe19897d5d8bd42ffcd5447e934a23
Patch Set 1 #Patch Set 2 : drop hasAudioCapabilities check #
Total comments: 14
Patch Set 3 : comments addressed and add UMA #
Total comments: 6
Patch Set 4 : comments addressed #
Total comments: 4
Patch Set 5 : comments addressed #
Messages
Total messages: 25 (6 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org
PTAL
https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:90: return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty()) I'm not certain that we should restrict to the size() == 1 case. I see three other options that make more sense: - Warn if the first returned capability has no robustness. - Warn if any returned capability has no robustness. - Warn if there are capabilities, but none of them have robustness. The last one is probably the right choice, since it's a close match for where behavior will change. https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:164: "It is recommended that a robustness level be specified. In a future version, Chromium will be made " While this message is the one recommended by ddorwin@, it seems overly long and doesn't have a clear message. In fact, the real result is that we may provide a *higher* robustness level after the change than we do now. I don't know what the proposed spec change is, so it's hard to recommend a precise wording.
https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:84: return keySystem == "com.widevine.alpha"; This should be in Chromium, though I don't know how easy it is to write to the console. Also, we wouldn't go to the trouble to abstract it correctly. Since this is temporary, it's probably fine. https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:90: return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty()) Empty might be legit for audio. For example, it's unclear what value to use if the audio is not encrypted. Empty string seems appropriate rather than requiring use of a minimal decryption capability. Thus, we should probably drop this one. WDYT? https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:90: return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty()) On 2015/10/01 20:40:24, sandersd wrote: > I'm not certain that we should restrict to the size() == 1 case. I see three > other options that make more sense: > - Warn if the first returned capability has no robustness. > - Warn if any returned capability has no robustness. > - Warn if there are capabilities, but none of them have robustness. > > The last one is probably the right choice, since it's a close match for where > behavior will change. Good point. My assumption was that the empty one would be last, but there's no guarantee of that. However, that is probably the _only_ case where we might want to not warn about the empty string. (To do this entirely correctly, we'd have to check the input array, not just the output, to see if we rejected subsequent entries.) The real question is whether the empty string should _ever_ be provided for video and Widevine. The only use case I can think of is supporting some legacy system that didn't implement robustness values. However, Chromium implemented these in ~M43, so I would expect most UAs to support them. Thus, perhaps we should just check for empty string in any entry. If that turns out to be too noisy (i.e. there is a legit reason of which we're not aware), we can always fix it. WDYT? https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:163: m_resolver->scriptState()->executionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, Are we guaranteed that the EC will be valid? https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:164: "It is recommended that a robustness level be specified. In a future version, Chromium will be made " I think we generally use "Chrome" in console messages. At least for deprecation messages in Blink. https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:164: "It is recommended that a robustness level be specified. In a future version, Chromium will be made " On 2015/10/01 20:40:24, sandersd wrote: > While this message is the one recommended by ddorwin@, it seems overly long and > doesn't have a clear message. In fact, the real result is that we may provide a > *higher* robustness level after the change than we do now. I don't know what the > proposed spec change is, so it's hard to recommend a precise wording. There aren't any plans to change the spec beyond what is there today: "The empty string indicates that any ability to decrypt and decode the content type is acceptable." Of course, suggestions welcome. I agree with your points, though, and the wording was just a suggestion. :) Perhaps the real message is that not specifying the robustness level could result in unexpected behavior, potentially including failure to play in future versions. Of course, what we really want is "you should specify a robustness along with each contentType." Those are both clearer messages, but I'm not sure how to shrink that message.
https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:90: return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty()) On 2015/10/01 21:23:47, ddorwin wrote: > Empty might be legit for audio. For example, it's unclear what value to use if > the audio is not encrypted. Empty string seems appropriate rather than requiring > use of a minimal decryption capability. Thus, we should probably drop this one. > WDYT? The ideal design for compatibility would be to list the empty capabilities in a separate configuration entirely, so we can reasonably expect that all accepted capabilities should have a robustness. That said, other browsers are broken and I don't know what gymnastics providers will actually implement. I agree that there are no obvious use cases for video without a robustness, but your audio use case makes sense to me and is even likely. You get to make the call on 'any' vs 'all' though, I have no data on which to make a call. (There may even be an argument for continuing to warn for the 'none' case, even after the change.) https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:164: "It is recommended that a robustness level be specified. In a future version, Chromium will be made " On 2015/10/01 21:23:47, ddorwin wrote: > On 2015/10/01 20:40:24, sandersd wrote: > > While this message is the one recommended by ddorwin@, it seems overly long > and > > doesn't have a clear message. In fact, the real result is that we may provide > a > > *higher* robustness level after the change than we do now. I don't know what > the > > proposed spec change is, so it's hard to recommend a precise wording. > > There aren't any plans to change the spec beyond what is there today: "The empty > string indicates that any ability to decrypt and decode the content type is > acceptable." Of course, suggestions welcome. > > I agree with your points, though, and the wording was just a suggestion. :) > Perhaps the real message is that not specifying the robustness level could > result in unexpected behavior, potentially including failure to play in future > versions. Of course, what we really want is "you should specify a robustness > along with each contentType." Those are both clearer messages, but I'm not sure > how to shrink that message. Perhaps we should just say that the behavior will change, without clarification.
PTAL again! https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:84: return keySystem == "com.widevine.alpha"; On 2015/10/01 21:23:47, ddorwin wrote: > This should be in Chromium, though I don't know how easy it is to write to the > console. Also, we wouldn't go to the trouble to abstract it correctly. Since > this is temporary, it's probably fine. Acknowledged. https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:90: return (config.audioCapabilities.size() == 1 && config.audioCapabilities[0].robustness.isEmpty()) On 2015/10/01 21:34:45, sandersd wrote: > On 2015/10/01 21:23:47, ddorwin wrote: > > Empty might be legit for audio. For example, it's unclear what value to use if > > the audio is not encrypted. Empty string seems appropriate rather than > requiring > > use of a minimal decryption capability. Thus, we should probably drop this > one. > > WDYT? > > The ideal design for compatibility would be to list the empty capabilities in a > separate configuration entirely, so we can reasonably expect that all accepted > capabilities should have a robustness. That said, other browsers are broken and > I don't know what gymnastics providers will actually implement. > > I agree that there are no obvious use cases for video without a robustness, but > your audio use case makes sense to me and is even likely. > > You get to make the call on 'any' vs 'all' though, I have no data on which to > make a call. (There may even be an argument for continuing to warn for the > 'none' case, even after the change.) I am going to do "any". https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:163: m_resolver->scriptState()->executionContext()->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, On 2015/10/01 21:23:47, ddorwin wrote: > Are we guaranteed that the EC will be valid? Good question. Actually I don't know. But we have a precedent at l.106. https://chromiumcodereview.appspot.com/1381463003/diff/20001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:164: "It is recommended that a robustness level be specified. In a future version, Chromium will be made " On 2015/10/01 21:34:45, sandersd wrote: > On 2015/10/01 21:23:47, ddorwin wrote: > > On 2015/10/01 20:40:24, sandersd wrote: > > > While this message is the one recommended by ddorwin@, it seems overly long > > and > > > doesn't have a clear message. In fact, the real result is that we may > provide > > a > > > *higher* robustness level after the change than we do now. I don't know what > > the > > > proposed spec change is, so it's hard to recommend a precise wording. > > > > There aren't any plans to change the spec beyond what is there today: "The > empty > > string indicates that any ability to decrypt and decode the content type is > > acceptable." Of course, suggestions welcome. > > > > I agree with your points, though, and the wording was just a suggestion. :) > > Perhaps the real message is that not specifying the robustness level could > > result in unexpected behavior, potentially including failure to play in future > > versions. Of course, what we really want is "you should specify a robustness > > along with each contentType." Those are both clearer messages, but I'm not > sure > > how to shrink that message. > > Perhaps we should just say that the behavior will change, without clarification. Updated the text...
https://codereview.chromium.org/1381463003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://codereview.chromium.org/1381463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:147: void MediaKeySystemAccessInitializer::requestSucceeded(WebContentDecryptionModuleAccess* access) Since we're not checking whether any were selected, we could do this in RMKSA? That would also be a little more useful to the developer since the message should be associated with the call. Is there a reason we wouldn't want to report this for an unsupported configuration? If we move it, we might need to cycle through multiple configurations. https://codereview.chromium.org/1381463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:160: Platform::current()->histogramEnumeration("Media.EME.Widevine.VideoCapability.EmptyRobustness", capability.robustness.isEmpty(), 2); We're recording every entry. Is that what we want? Don't we just want to know the result of hasEmptyRobustness after the loop and record that? https://codereview.chromium.org/1381463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:160: Platform::current()->histogramEnumeration("Media.EME.Widevine.VideoCapability.EmptyRobustness", capability.robustness.isEmpty(), 2); Similarly, HasEmptyRobustness as the UMA name? The first comment might change the name as well.
https://chromiumcodereview.appspot.com/1381463003/diff/40001/third_party/WebK... File third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp (right): https://chromiumcodereview.appspot.com/1381463003/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:147: void MediaKeySystemAccessInitializer::requestSucceeded(WebContentDecryptionModuleAccess* access) On 2015/10/01 23:16:36, ddorwin wrote: > Since we're not checking whether any were selected, we could do this in RMKSA? > That would also be a little more useful to the developer since the message > should be associated with the call. > > Is there a reason we wouldn't want to report this for an unsupported > configuration? From the email discussion, I thought we want to limit the noise as much as possible. That why we choose to warn on the returned configuration. > If we move it, we might need to cycle through multiple configurations. Moved. https://chromiumcodereview.appspot.com/1381463003/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:160: Platform::current()->histogramEnumeration("Media.EME.Widevine.VideoCapability.EmptyRobustness", capability.robustness.isEmpty(), 2); On 2015/10/01 23:16:36, ddorwin wrote: > We're recording every entry. Is that what we want? Don't we just want to know > the result of hasEmptyRobustness after the loop and record that? Done. https://chromiumcodereview.appspot.com/1381463003/diff/40001/third_party/WebK... third_party/WebKit/Source/modules/encryptedmedia/NavigatorRequestMediaKeySystemAccess.cpp:160: Platform::current()->histogramEnumeration("Media.EME.Widevine.VideoCapability.EmptyRobustness", capability.robustness.isEmpty(), 2); On 2015/10/01 23:16:36, ddorwin wrote: > Similarly, HasEmptyRobustness as the UMA name? > > The first comment might change the name as well. Done.
PTAL again
LGTM. Possible nit. https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:17533: + specific to the Widevine key system. nit: This last sentence is redundant with the name, right?
xhwang@chromium.org changed reviewers: + isherman@chromium.org
isherman: Please OWNERS review histograms.xml
https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:17533: + specific to the Widevine key system. On 2015/10/02 01:19:04, ddorwin wrote: > nit: This last sentence is redundant with the name, right? Well, we do mention Widevine in other Media.EME.Widevine.* UMAs.
histograms lgtm % a nit: https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:17528: + enum="Boolean"> nit: Please define a custom boolean enum, BooleanEmpty
comments addressed
https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/1381463003/diff/60001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:17528: + enum="Boolean"> On 2015/10/02 04:14:12, Ilya Sherman wrote: > nit: Please define a custom boolean enum, BooleanEmpty Done.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, ddorwin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1381463003/#ps80001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381463003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381463003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381463003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381463003/80001
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0f9748c2c6fe19897d5d8bd42ffcd5447e934a23 Cr-Commit-Position: refs/heads/master@{#352118}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 0f9748c2c6fe19897d5d8bd42ffcd5447e934a23 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
