Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
58933003:
media: VP9 MediaSource Support in Clank (Closed)
|
Created:
7 years, 1 month ago by vignesh Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionmedia: VP9 MediaSource Support in Clank
Adding a bunch of missing switch cases for VP9. Also fixing
isTypeSupported to return true for VP9 in Clank depending upon the
underlying android version.
BUG=285016
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234832
Patch Set 1 #
Total comments: 6
Patch Set 2 : hardcoding android api level check for vp9 #
Total comments: 4
Patch Set 3 : nits #
Messages
Total messages: 20 (0 generated)
On 2013/11/08 22:46:18, vignesh wrote: Tests?
https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:84: CodecInfo::HISTOGRAM_OPUS }; I think Opus needs to be shut off for OS_ANDROID.
https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:279: if (base::android::BuildInfo::GetInstance()->sdk_int() < I'd almost consider hardcoding VP9 + SDK support here instead of adding an extra field to the struct :\ acolwell: thoughts?
+acolwell (I have a question for you)
https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:279: if (base::android::BuildInfo::GetInstance()->sdk_int() < On 2013/11/09 00:29:34, scherkus wrote: > I'd almost consider hardcoding VP9 + SDK support here instead of adding an extra > field to the struct :\ > > acolwell: thoughts? I agree that hardcoding is a better path than modifying the struct. That way it keeps all of the necessary Android specific changes right here.
On 2013/11/09 00:43:14, acolwell wrote: > https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... > File media/filters/stream_parser_factory.cc (right): > > https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... > media/filters/stream_parser_factory.cc:279: if > (base::android::BuildInfo::GetInstance()->sdk_int() < > On 2013/11/09 00:29:34, scherkus wrote: > > I'd almost consider hardcoding VP9 + SDK support here instead of adding an > extra > > field to the struct :\ > > > > acolwell: thoughts? > > I agree that hardcoding is a better path than modifying the struct. That way it > keeps all of the necessary Android specific changes right here. acolwell: you'll probably want to look at https://codereview.chromium.org/66863003/ as well
https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:84: CodecInfo::HISTOGRAM_OPUS }; On 2013/11/08 23:46:04, fgalligan1 wrote: > I think Opus needs to be shut off for OS_ANDROID. will do this in a separate CL. https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:279: if (base::android::BuildInfo::GetInstance()->sdk_int() < On 2013/11/09 00:43:14, acolwell wrote: > On 2013/11/09 00:29:34, scherkus wrote: > > I'd almost consider hardcoding VP9 + SDK support here instead of adding an > extra > > field to the struct :\ > > > > acolwell: thoughts? > > I agree that hardcoding is a better path than modifying the struct. That way it > keeps all of the necessary Android specific changes right here. We will definitely end up having more codecs which are android version dependent (opus, 265 in the near future). So are you sure you want me to hard code vp9 here instead of using this generic extension? Hard coding might get messy when we have more codecs start to depend on different android versions.
On 2013/11/11 16:00:43, vignesh wrote: > https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... > File media/filters/stream_parser_factory.cc (right): > > https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... > media/filters/stream_parser_factory.cc:84: CodecInfo::HISTOGRAM_OPUS }; > On 2013/11/08 23:46:04, fgalligan1 wrote: > > I think Opus needs to be shut off for OS_ANDROID. > > will do this in a separate CL. > > https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... > media/filters/stream_parser_factory.cc:279: if > (base::android::BuildInfo::GetInstance()->sdk_int() < > > On 2013/11/09 00:43:14, acolwell wrote: > > On 2013/11/09 00:29:34, scherkus wrote: > > > I'd almost consider hardcoding VP9 + SDK support here instead of adding an > > extra > > > field to the struct :\ > > > > > > acolwell: thoughts? > > > > I agree that hardcoding is a better path than modifying the struct. That way > it > > keeps all of the necessary Android specific changes right here. > > We will definitely end up having more codecs which are android version dependent > (opus, 265 in the near future). So are you sure you want me to hard code vp9 > here instead of using this generic extension? Hard coding might get messy when > we have more codecs start to depend on different android versions. Yes. I want to hard code it for now. We will add the generic extension when we have more than one instance of this. It is clear from these CLs that we may need to rethink how we are handling exposing codec support to isTypeSupported() & canPlayType(), but that is a separate issue from what you are trying to accomplish here.
https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:279: if (base::android::BuildInfo::GetInstance()->sdk_int() < As mentioned in the other comment, hardcoding VP9. On 2013/11/11 16:00:43, vignesh wrote: > > On 2013/11/09 00:43:14, acolwell wrote: > > On 2013/11/09 00:29:34, scherkus wrote: > > > I'd almost consider hardcoding VP9 + SDK support here instead of adding an > > extra > > > field to the struct :\ > > > > > > acolwell: thoughts? > > > > I agree that hardcoding is a better path than modifying the struct. That way > it > > keeps all of the necessary Android specific changes right here. > > We will definitely end up having more codecs which are android version dependent > (opus, 265 in the near future). So are you sure you want me to hard code vp9 > here instead of using this generic extension? Hard coding might get messy when > we have more codecs start to depend on different android versions.
lgtm w/ nits thanks for making the change! https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... media/filters/stream_parser_factory.cc:273: // VP9 is only supported on KitKat+ (API Level 19) comments end w/ periods https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... media/filters/stream_parser_factory.cc:276: return false; nit: ifs > 1 line need { }
https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... media/filters/stream_parser_factory.cc:273: // VP9 is only supported on KitKat+ (API Level 19) On 2013/11/11 19:00:44, scherkus wrote: > comments end w/ periods Done. https://codereview.chromium.org/58933003/diff/120001/media/filters/stream_par... media/filters/stream_parser_factory.cc:276: return false; On 2013/11/11 19:00:44, scherkus wrote: > nit: ifs > 1 line need { } Done.
On 2013/11/08 23:45:38, fgalligan1 wrote: > On 2013/11/08 22:46:18, vignesh wrote: > > Tests? I have no idea how to test this. Are there android specific layout tests ? qinmin@ ?
On 2013/11/11 19:16:47, vignesh wrote: > On 2013/11/08 23:45:38, fgalligan1 wrote: > > On 2013/11/08 22:46:18, vignesh wrote: > > > > Tests? > > I have no idea how to test this. Are there android specific layout tests ? > qinmin@ ? There is one bot for layout tests: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexus4) I am not sure whether that bot has K to test your patch, you need to ping beverloo@ about this. However, You can always run your test manually on your local machine, and making sure it works as expected in both K and prior-K devices
On 2013/11/11 19:39:45, qinmin wrote: > On 2013/11/11 19:16:47, vignesh wrote: > > On 2013/11/08 23:45:38, fgalligan1 wrote: > > > On 2013/11/08 22:46:18, vignesh wrote: > > > > > > Tests? > > > > I have no idea how to test this. Are there android specific layout tests ? > > qinmin@ ? > > There is one bot for layout tests: > http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Android%2520%...) > I am not sure whether that bot has K to test your patch, you need to ping > beverloo@ about this. > However, You can always run your test manually on your local machine, and making > sure it works as expected in both K and prior-K devices I have already verified this manually in both K and prior-K versions.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/58933003/120002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/58933003/120002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/58933003/120002
Message was sent while issue was closed.
Change committed as 234832 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
