Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27230004:
Do not run MSE related tests when MSE is not available. (Closed)
|
Created:
7 years, 2 months ago by xhwang Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, wjia+watch_chromium.org, darin-cc_chromium.org, sky Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not run MSE related tests when MSE is not available.
BUG=304956
TEST=This make MSE related tests doesn't run on unsupported platform/devices.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229610
Patch Set 1 #
Total comments: 11
Patch Set 2 : Check MediaCodecBridge Availability #
Total comments: 6
Patch Set 3 : Address Comments #
Total comments: 6
Patch Set 4 : Address Comments #
Total comments: 2
Patch Set 5 : rebase only #
Messages
Total messages: 20 (0 generated)
ddorwin/qinmin/sky: This CL violated DEPS rule so it's not ready for review. Please see my question :) https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:14: #include "third_party/WebKit/public/web/WebRuntimeFeatures.h" This violates DEPS rules [1]. But since we set WebRuntimeFeatures directly without using any flag [2], I don't see a better way to do this without checking Android version directly (which IMHO is more hacky). Thoughts? [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt...
https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:14: #include "third_party/WebKit/public/web/WebRuntimeFeatures.h" Maybe add a function IsMSESupported() somewhere in content? And then both the runtime_features and this file can use that function? Anyway, I don't see a way to get around the android version check, the WebRuntimeFeatures for MSE are still determined from the android version On 2013/10/14 20:12:47, xhwang wrote: > This violates DEPS rules [1]. But since we set WebRuntimeFeatures directly > without using any flag [2], I don't see a better way to do this without checking > Android version directly (which IMHO is more hacky). Thoughts? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE... > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... >
https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:14: #include "third_party/WebKit/public/web/WebRuntimeFeatures.h" On 2013/10/14 20:59:45, qinmin wrote: > Maybe add a function IsMSESupported() somewhere in content? > And then both the runtime_features and this file can use that function? > Anyway, I don't see a way to get around the android version check, the > WebRuntimeFeatures for MSE are still determined from the android version > > On 2013/10/14 20:12:47, xhwang wrote: > > This violates DEPS rules [1]. But since we set WebRuntimeFeatures directly > > without using any flag [2], I don't see a better way to do this without > checking > > Android version directly (which IMHO is more hacky). Thoughts? > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE... > > > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... > > > I would rather run the tests unless we explicitly know the platform does not support the tests. If someone accidentally flipped the runtime feature flag, we'd lose coverage and not know about it. The actual check [3] is pretty simple and permanent, so I think it's fine to include here. For simplicity, wrap it in a helper like qinmin suggested. A name that references platform limitations might be better, but I can't think of a good one. [3] https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:105: // TODO(xhwang): Update this to check for unprefixed MSE API when A helper function also allows this to be encapsulated in one place. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:133: if (src_type == MSE && Where do we prevent SRC tests from running on Android? It doesn't look like we do (the content test has it).
Check MediaCodecBridge Availability
This still involves a DEPS change. I can revert the change in runtime_features.cc to avoid the DEPS change, but I feel checking MediaCodecBridge directly is much better than checking android version. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:14: #include "third_party/WebKit/public/web/WebRuntimeFeatures.h" On 2013/10/14 21:30:34, ddorwin wrote: > On 2013/10/14 20:59:45, qinmin wrote: > > Maybe add a function IsMSESupported() somewhere in content? > > And then both the runtime_features and this file can use that function? > > Anyway, I don't see a way to get around the android version check, the > > WebRuntimeFeatures for MSE are still determined from the android version > > > > On 2013/10/14 20:12:47, xhwang wrote: > > > This violates DEPS rules [1]. But since we set WebRuntimeFeatures directly > > > without using any flag [2], I don't see a better way to do this without > > checking > > > Android version directly (which IMHO is more hacky). Thoughts? > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE... > > > > > > [2] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... > > > > > > > I would rather run the tests unless we explicitly know the platform does not > support the tests. If someone accidentally flipped the runtime feature flag, > we'd lose coverage and not know about it. > > The actual check [3] is pretty simple and permanent, so I think it's fine to > include here. For simplicity, wrap it in a helper like qinmin suggested. A name > that references platform limitations might be better, but I can't think of a > good one. > > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... Instead of checking android version, we can just check MediaCodecBridge's availability which is the first hand information about whether MSE is available on Android. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:105: // TODO(xhwang): Update this to check for unprefixed MSE API when On 2013/10/14 21:30:34, ddorwin wrote: > A helper function also allows this to be encapsulated in one place. Done. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:133: if (src_type == MSE && On 2013/10/14 21:30:34, ddorwin wrote: > Where do we prevent SRC tests from running on Android? It doesn't look like we > do (the content test has it). I only fixed content_browsertest and haven't fixed browser_test yet. Currently browser_tests doesn't run on Android at all so this is low priority.
https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:14: #include "third_party/WebKit/public/web/WebRuntimeFeatures.h" On 2013/10/16 07:58:40, xhwang wrote: > On 2013/10/14 21:30:34, ddorwin wrote: > > On 2013/10/14 20:59:45, qinmin wrote: > > > Maybe add a function IsMSESupported() somewhere in content? > > > And then both the runtime_features and this file can use that function? > > > Anyway, I don't see a way to get around the android version check, the > > > WebRuntimeFeatures for MSE are still determined from the android version > > > > > > On 2013/10/14 20:12:47, xhwang wrote: > > > > This violates DEPS rules [1]. But since we set WebRuntimeFeatures directly > > > > without using any flag [2], I don't see a better way to do this without > > > checking > > > > Android version directly (which IMHO is more hacky). Thoughts? > > > > > > > > [1] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/DE... > > > > > > > > [2] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... > > > > > > > > > > > I would rather run the tests unless we explicitly know the platform does not > > support the tests. If someone accidentally flipped the runtime feature flag, > > we'd lose coverage and not know about it. > > > > The actual check [3] is pretty simple and permanent, so I think it's fine to > > include here. For simplicity, wrap it in a helper like qinmin suggested. A > name > > that references platform limitations might be better, but I can't think of a > > good one. > > > > [3] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/runt... > > Instead of checking android version, we can just check MediaCodecBridge's > availability which is the first hand information about whether MSE is available > on Android. If the MediaCodecBridge availability code breaks, though, you won't know because the test relies on it. It's better to avoid relying on product code to drive tests. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:133: if (src_type == MSE && On 2013/10/16 07:58:40, xhwang wrote: > On 2013/10/14 21:30:34, ddorwin wrote: > > Where do we prevent SRC tests from running on Android? It doesn't look like we > > do (the content test has it). > > I only fixed content_browsertest and haven't fixed browser_test yet. Currently > browser_tests doesn't run on Android at all so this is low priority. Should we drop the MSE check then? It seems EME would always rely on MediaCodecs. https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_browsertest.cc:108: if (!media::MediaCodecBridge::IsAvailable()) { As mentioned in PS1, I don't think we should rely on product code for this decision. https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_browsertest.cc:109: LOG(INFO) << "Could not run test - MSE not supported."; An explicit check also allows you to say why here (e.g. pre-JB). https://codereview.chromium.org/27230004/diff/7001/content/child/runtime_feat... File content/child/runtime_features.cc (right): https://codereview.chromium.org/27230004/diff/7001/content/child/runtime_feat... content/child/runtime_features.cc:24: if (!media::MediaCodecBridge::IsAvailable()) { I like this change!
Address Comments
PTAL again. https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/1/chrome/browser/media/encrypte... chrome/browser/media/encrypted_media_browsertest.cc:133: if (src_type == MSE && On 2013/10/16 20:17:17, ddorwin wrote: > On 2013/10/16 07:58:40, xhwang wrote: > > On 2013/10/14 21:30:34, ddorwin wrote: > > > Where do we prevent SRC tests from running on Android? It doesn't look like > we > > > do (the content test has it). > > > > I only fixed content_browsertest and haven't fixed browser_test yet. Currently > > browser_tests doesn't run on Android at all so this is low priority. > > Should we drop the MSE check then? It seems EME would always rely on > MediaCodecs. Done. I also fixed browser_tests to support Android. Not sure if I should do that in a separate CL. PTAL. https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_browsertest.cc:108: if (!media::MediaCodecBridge::IsAvailable()) { On 2013/10/16 20:17:17, ddorwin wrote: > As mentioned in PS1, I don't think we should rely on product code for this > decision. Done. https://codereview.chromium.org/27230004/diff/7001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_browsertest.cc:109: LOG(INFO) << "Could not run test - MSE not supported."; On 2013/10/16 20:17:17, ddorwin wrote: > An explicit check also allows you to say why here (e.g. pre-JB). Done. https://codereview.chromium.org/27230004/diff/7001/content/child/runtime_feat... File content/child/runtime_features.cc (right): https://codereview.chromium.org/27230004/diff/7001/content/child/runtime_feat... content/child/runtime_features.cc:24: if (!media::MediaCodecBridge::IsAvailable()) { On 2013/10/16 20:17:17, ddorwin wrote: > I like this change! :)
https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_browsertest.cc:108: if (base::android::BuildInfo::GetInstance()->sdk_int() < 16) { make this a static boolean function at the top of the file? So you won't need to have the same ifdefs later. https://codereview.chromium.org/27230004/diff/20001/content/child/DEPS File content/child/DEPS (right): https://codereview.chromium.org/27230004/diff/20001/content/child/DEPS#newcode4 content/child/DEPS:4: "+media/base/android", better ask some content/ OWNERs to see if this include is ok
jam@: PTAL at content/child/*. This new check is more accurate and less hacky than the old one.
lgtm % nit https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_browsertest.cc:109: LOG(INFO) << "Skipping test - MSE only supported in JellyBean and later."; JB is two words. Maybe just say 4.1?
lgtm
Address Comments
ddorwin/qinmin: The change is not trivial. PTAL again. https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_browsertest.cc:108: if (base::android::BuildInfo::GetInstance()->sdk_int() < 16) { On 2013/10/16 22:14:34, qinmin wrote: > make this a static boolean function at the top of the file? So you won't need to > have the same ifdefs later. Done. https://codereview.chromium.org/27230004/diff/20001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_browsertest.cc:109: LOG(INFO) << "Skipping test - MSE only supported in JellyBean and later."; On 2013/10/16 22:51:46, ddorwin wrote: > JB is two words. Maybe just say 4.1? I tried 4.1 but in the context I need to use "Android 4.1" which is even longer. https://codereview.chromium.org/27230004/diff/20001/content/child/DEPS File content/child/DEPS (right): https://codereview.chromium.org/27230004/diff/20001/content/child/DEPS#newcode4 content/child/DEPS:4: "+media/base/android", On 2013/10/16 22:14:34, qinmin wrote: > better ask some content/ OWNERs to see if this include is ok Done and got l'g't'm. https://codereview.chromium.org/27230004/diff/32001/build/android/pylib/gtest... File build/android/pylib/gtest/filter/content_browsertests_disabled (right): https://codereview.chromium.org/27230004/diff/32001/build/android/pylib/gtest... build/android/pylib/gtest/filter/content_browsertests_disabled:10: *EncryptedMediaTest.* I'll enable this in a later CL, which will be easier to revert :)
lgtm % nit https://codereview.chromium.org/27230004/diff/32001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/27230004/diff/32001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_browsertest.cc:66: LOG(INFO) << "MSE is only supported in JellyBean and later."; You have more room to add a space and even 4.1. :)
qinmin: ping
lgtm. Sorry, i thought I lgtm'ed this
rebase only
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/27230004/45001
Message was sent while issue was closed.
Change committed as 229610 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
