Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
66863003:
Add VP9 to canPlayType on Android (Closed)
|
Created:
7 years, 1 month ago by vignesh Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptioncanPlayType could return maybe/probably for VP9 in android
VP9 support was added to android in Kitkat. Changing canPlayType to return
probably/maybe if running on kitkat or higher.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234701
Patch Set 1 #Patch Set 2 : minor fix. please ignore ps1. #
Total comments: 7
Patch Set 3 : hardcoding vp9 android version check #
Total comments: 8
Patch Set 4 : addressing comments #Messages
Total messages: 26 (0 generated)
Have modified mime_util to generalize checking ability to play various codecs/containers in android. Right now vp9 is the only special case (as it is supported only in kitkat+). The logic can be reused when we add more codecs in the future (like opus, h265, etc.).
On 2013/11/08 18:35:33, vignesh wrote: > Have modified mime_util to generalize checking ability to play various > codecs/containers in android. Right now vp9 is the only special case (as it is > supported only in kitkat+). The logic can be reused when we add more codecs in > the future (like opus, h265, etc.). The description is kind of weird. Maybe "Add VP9 to canPlayType on Android."? Tests?
looks like we should update the following layout test: http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/media-can-play-w... http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/media-can-play-w... https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:11: #include "base/android/build_info.h" typically #if #includes are put after the first block of #includes (i.e,. starting on line 23) https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. it seems unfortunate to duplicate both lists and minimum API levels for codecs what about something like the following: struct AndroidCodecRequirements { const char* codec; const int minimum_api_level; }; // If codec not present then there are no requirements. static const AndroidCodecRequirements android_codec_requirements[] = { { "vp9", 19 } }; then when filtering through the common_media_codecs and format_codec_mappings, we check for the presence and passing of the requirements
https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:493: for (size_t i = 0; i < arraysize(common_media_codecs); ++i) we'll also need { }s for this for loop and one below
+acolwell
https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. On 2013/11/09 00:20:05, scherkus wrote: > it seems unfortunate to duplicate both lists and minimum API levels for codecs > > what about something like the following: > > struct AndroidCodecRequirements { > const char* codec; > const int minimum_api_level; > }; > > // If codec not present then there are no requirements. > static const AndroidCodecRequirements android_codec_requirements[] = { > { "vp9", 19 } > }; > > then when filtering through the common_media_codecs and format_codec_mappings, > we check for the presence and passing of the requirements if duplication is your only concern, then how about getting rid of it this way? I leave the struct as it is now in my CL. The initialization can be done like this: // if there are no requirements, min_api_level is left uninitialized (thereby automatically initializing them to zero). ... { "vorbis" }, { "vp8" }, { "vp9" #if defined(OS_ANDROID) , 19 #endif } ...
On 2013/11/11 16:07:10, vignesh wrote: > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... > net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. > On 2013/11/09 00:20:05, scherkus wrote: > > it seems unfortunate to duplicate both lists and minimum API levels for codecs > > > > what about something like the following: > > > > struct AndroidCodecRequirements { > > const char* codec; > > const int minimum_api_level; > > }; > > > > // If codec not present then there are no requirements. > > static const AndroidCodecRequirements android_codec_requirements[] = { > > { "vp9", 19 } > > }; > > > > then when filtering through the common_media_codecs and format_codec_mappings, > > we check for the presence and passing of the requirements > > if duplication is your only concern, then how about getting rid of it this way? > > I leave the struct as it is now in my CL. The initialization can be done like > this: > > // if there are no requirements, min_api_level is left uninitialized (thereby > automatically initializing them to zero). > ... > { "vorbis" }, > { "vp8" }, > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > ... I prefer explicit code ... while it's handy to rely on the compiler to initialize missing values to zero, I believe it'd be even better to avoid checking min_api_level on platforms where such a concept doesn't exist.
On 2013/11/11 18:39:05, scherkus wrote: > On 2013/11/11 16:07:10, vignesh wrote: > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc > > File net/base/mime_util.cc (right): > > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... > > net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. > > On 2013/11/09 00:20:05, scherkus wrote: > > > it seems unfortunate to duplicate both lists and minimum API levels for > codecs > > > > > > what about something like the following: > > > > > > struct AndroidCodecRequirements { > > > const char* codec; > > > const int minimum_api_level; > > > }; > > > > > > // If codec not present then there are no requirements. > > > static const AndroidCodecRequirements android_codec_requirements[] = { > > > { "vp9", 19 } > > > }; > > > > > > then when filtering through the common_media_codecs and > format_codec_mappings, > > > we check for the presence and passing of the requirements > > > > if duplication is your only concern, then how about getting rid of it this > way? > > > > I leave the struct as it is now in my CL. The initialization can be done like > > this: > > > > // if there are no requirements, min_api_level is left uninitialized (thereby > > automatically initializing them to zero). > > ... > > { "vorbis" }, > > { "vp8" }, > > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > > ... > > I prefer explicit code ... while it's handy to rely on the compiler to > initialize missing values to zero, I believe it'd be even better to avoid > checking min_api_level on platforms where such a concept doesn't exist. Sorry if i was not clear before, The existence of min_api_level in the struct is still ifdef'd to be present only on android. So we won't be checking min_api_level on other platforms.
On 2013/11/11 18:42:58, vignesh wrote: > On 2013/11/11 18:39:05, scherkus wrote: > > On 2013/11/11 16:07:10, vignesh wrote: > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc > > > File net/base/mime_util.cc (right): > > > > > > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... > > > net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. > > > On 2013/11/09 00:20:05, scherkus wrote: > > > > it seems unfortunate to duplicate both lists and minimum API levels for > > codecs > > > > > > > > what about something like the following: > > > > > > > > struct AndroidCodecRequirements { > > > > const char* codec; > > > > const int minimum_api_level; > > > > }; > > > > > > > > // If codec not present then there are no requirements. > > > > static const AndroidCodecRequirements android_codec_requirements[] = { > > > > { "vp9", 19 } > > > > }; > > > > > > > > then when filtering through the common_media_codecs and > > format_codec_mappings, > > > > we check for the presence and passing of the requirements > > > > > > if duplication is your only concern, then how about getting rid of it this > > way? > > > > > > I leave the struct as it is now in my CL. The initialization can be done > like > > > this: > > > > > > // if there are no requirements, min_api_level is left uninitialized > (thereby > > > automatically initializing them to zero). > > > ... > > > { "vorbis" }, > > > { "vp8" }, > > > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > > > ... > > > > I prefer explicit code ... while it's handy to rely on the compiler to > > initialize missing values to zero, I believe it'd be even better to avoid > > checking min_api_level on platforms where such a concept doesn't exist. > > Sorry if i was not clear before, The existence of min_api_level in the struct is > still ifdef'd to be present only on android. So we won't be checking > min_api_level on other platforms. I see ... I'm still not a fan of using #ifdefs to insert an Android-specific field. At that point I'd wonder why we can't split things out into a separate struct OR go the https://codereview.chromium.org/58933003/ route and just hardcode VP9 + SDK level for now until we have a need for something more sophisticated.
On 2013/11/11 18:59:43, scherkus wrote: > On 2013/11/11 18:42:58, vignesh wrote: > > On 2013/11/11 18:39:05, scherkus wrote: > > > On 2013/11/11 16:07:10, vignesh wrote: > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc > > > > File net/base/mime_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... > > > > net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in > Kitkat+. > > > > On 2013/11/09 00:20:05, scherkus wrote: > > > > > it seems unfortunate to duplicate both lists and minimum API levels for > > > codecs > > > > > > > > > > what about something like the following: > > > > > > > > > > struct AndroidCodecRequirements { > > > > > const char* codec; > > > > > const int minimum_api_level; > > > > > }; > > > > > > > > > > // If codec not present then there are no requirements. > > > > > static const AndroidCodecRequirements android_codec_requirements[] = { > > > > > { "vp9", 19 } > > > > > }; > > > > > > > > > > then when filtering through the common_media_codecs and > > > format_codec_mappings, > > > > > we check for the presence and passing of the requirements > > > > > > > > if duplication is your only concern, then how about getting rid of it this > > > way? > > > > > > > > I leave the struct as it is now in my CL. The initialization can be done > > like > > > > this: > > > > > > > > // if there are no requirements, min_api_level is left uninitialized > > (thereby > > > > automatically initializing them to zero). > > > > ... > > > > { "vorbis" }, > > > > { "vp8" }, > > > > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > > > > ... > > > > > > I prefer explicit code ... while it's handy to rely on the compiler to > > > initialize missing values to zero, I believe it'd be even better to avoid > > > checking min_api_level on platforms where such a concept doesn't exist. > > > > Sorry if i was not clear before, The existence of min_api_level in the struct > is > > still ifdef'd to be present only on android. So we won't be checking > > min_api_level on other platforms. > > I see ... I'm still not a fan of using #ifdefs to insert an Android-specific > field. At that point I'd wonder why we can't split things out into a separate > struct OR go the https://codereview.chromium.org/58933003/ route and just > hardcode VP9 + SDK level for now until we have a need for something more > sophisticated. I was thinking the same too. If we are hardcoding isTypeSupported, we should hardcode this too for now. Also as acolwell mentioned in the other CL's comments, it is probably time for us to rethink how this whole canPlayType and isTypeSupported is handled. :) Anyway, i will go ahead and hardcode it here for now.
On 2013/11/11 19:05:40, vignesh wrote: > On 2013/11/11 18:59:43, scherkus wrote: > > On 2013/11/11 18:42:58, vignesh wrote: > > > On 2013/11/11 18:39:05, scherkus wrote: > > > > On 2013/11/11 16:07:10, vignesh wrote: > > > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc > > > > > File net/base/mime_util.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... > > > > > net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in > > Kitkat+. > > > > > On 2013/11/09 00:20:05, scherkus wrote: > > > > > > it seems unfortunate to duplicate both lists and minimum API levels > for > > > > codecs > > > > > > > > > > > > what about something like the following: > > > > > > > > > > > > struct AndroidCodecRequirements { > > > > > > const char* codec; > > > > > > const int minimum_api_level; > > > > > > }; > > > > > > > > > > > > // If codec not present then there are no requirements. > > > > > > static const AndroidCodecRequirements android_codec_requirements[] = { > > > > > > { "vp9", 19 } > > > > > > }; > > > > > > > > > > > > then when filtering through the common_media_codecs and > > > > format_codec_mappings, > > > > > > we check for the presence and passing of the requirements > > > > > > > > > > if duplication is your only concern, then how about getting rid of it > this > > > > way? > > > > > > > > > > I leave the struct as it is now in my CL. The initialization can be done > > > like > > > > > this: > > > > > > > > > > // if there are no requirements, min_api_level is left uninitialized > > > (thereby > > > > > automatically initializing them to zero). > > > > > ... > > > > > { "vorbis" }, > > > > > { "vp8" }, > > > > > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > > > > > ... > > > > > > > > I prefer explicit code ... while it's handy to rely on the compiler to > > > > initialize missing values to zero, I believe it'd be even better to avoid > > > > checking min_api_level on platforms where such a concept doesn't exist. > > > > > > Sorry if i was not clear before, The existence of min_api_level in the > struct > > is > > > still ifdef'd to be present only on android. So we won't be checking > > > min_api_level on other platforms. > > > > I see ... I'm still not a fan of using #ifdefs to insert an Android-specific > > field. At that point I'd wonder why we can't split things out into a separate > > struct OR go the https://codereview.chromium.org/58933003/ route and just > > hardcode VP9 + SDK level for now until we have a need for something more > > sophisticated. > > I was thinking the same too. If we are hardcoding isTypeSupported, we should > hardcode this too for now. Also as acolwell mentioned in the other CL's > comments, it is probably time for us to rethink how this whole canPlayType and > isTypeSupported is handled. :) Anyway, i will go ahead and hardcode it here for > now. SGTM
I will also update the layout test shortly. Although, the layout tests will not test the exact behavior that this CL is trying to introduce, it will merely add a canPlayType check for vp9 in desktop. I have manually tested the android specific behavior in K and prior-K versions. https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:11: #include "base/android/build_info.h" On 2013/11/09 00:20:05, scherkus wrote: > typically #if #includes are put after the first block of #includes (i.e,. > starting on line 23) Done. https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:326: { "vp9", 19 }, // VP9 supported only in Kitkat+. On 2013/11/11 16:07:10, vignesh wrote: > On 2013/11/09 00:20:05, scherkus wrote: > > it seems unfortunate to duplicate both lists and minimum API levels for codecs > > > > what about something like the following: > > > > struct AndroidCodecRequirements { > > const char* codec; > > const int minimum_api_level; > > }; > > > > // If codec not present then there are no requirements. > > static const AndroidCodecRequirements android_codec_requirements[] = { > > { "vp9", 19 } > > }; > > > > then when filtering through the common_media_codecs and format_codec_mappings, > > we check for the presence and passing of the requirements > > if duplication is your only concern, then how about getting rid of it this way? > > I leave the struct as it is now in my CL. The initialization can be done like > this: > > // if there are no requirements, min_api_level is left uninitialized (thereby > automatically initializing them to zero). > ... > { "vorbis" }, > { "vp8" }, > { "vp9" #if defined(OS_ANDROID) , 19 #endif } > ... Hardcoding this as discussed on other comments. https://codereview.chromium.org/66863003/diff/40001/net/base/mime_util.cc#new... net/base/mime_util.cc:493: for (size_t i = 0; i < arraysize(common_media_codecs); ++i) On 2013/11/09 00:31:00, scherkus wrote: > we'll also need { }s for this for loop and one below Done.
CL for updating the layout test: https://codereview.chromium.org/69453002/
https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:416: static bool IsCodecSupportedOnAndroid(std::string codec) { const-ref https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:483: if (IsCodecSupportedOnAndroid(common_media_codecs[i])) technically line 485 should be indented as this if statement is applying to it ... it's really subtle when reading I'd reverse the condition and make the android case use a continue statement https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:502: if (IsCodecSupportedOnAndroid(mime_type_codecs[j])) ditto
https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:420: return false; you need {} as the if statement contains multiple lines.
https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:416: static bool IsCodecSupportedOnAndroid(std::string codec) { On 2013/11/11 21:33:46, scherkus wrote: > const-ref Done. https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:420: return false; On 2013/11/11 21:52:50, qinmin wrote: > you need {} as the if statement contains multiple lines. Done. https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:483: if (IsCodecSupportedOnAndroid(common_media_codecs[i])) On 2013/11/11 21:33:46, scherkus wrote: > technically line 485 should be indented as this if statement is applying to it > ... it's really subtle when reading > > I'd reverse the condition and make the android case use a continue statement Nice idea. Done. https://codereview.chromium.org/66863003/diff/170001/net/base/mime_util.cc#ne... net/base/mime_util.cc:502: if (IsCodecSupportedOnAndroid(mime_type_codecs[j])) On 2013/11/11 21:33:46, scherkus wrote: > ditto Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/66863003/240001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
asanka@, could you please do an OWNER's review of this. It's small change and should be quick LGTM'able. Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/66863003/240001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/66863003/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/66863003/240001
Message was sent while issue was closed.
Change committed as 234701 |
