Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
418193003:
Using PROFILE_ANY for vp8 and vp9 (Closed)
|
Created:
6 years, 4 months ago by amogh.bihani Modified:
6 years, 4 months ago Reviewers:
bbudge, fgalligan1, xhwang, dmichael (off chromium), bbudge-google, noelallen1, Pawel Osciak CC:
chromium-reviews, hclam+watch_chromium.org, imcheng+watch_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, pwestin+watch_google.com, feature-media-reviews_chromium.org, hguihot+watch_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, piman+watch_chromium.org, miu+watch_chromium.org, wjia+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org, bankoski, Yaowu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUsing PROFILE_ANY for vp8 and vp9
VP8 and VP9 do not take profile into account. Using PROFILE_MAIN is confusing.
This patch uses PROFILE_ANY for these codecs.
TBR=noelallen@chromium.org
BUG=361676
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289765
Patch Set 1 #
Total comments: 12
Patch Set 2 : Changing v8/9 profile name in pp_codec #Patch Set 3 : Renaming not_needed to not_specified #Patch Set 4 : fixing compile failures #
Total comments: 5
Patch Set 5 : Changing unpecified to not_applicable #Patch Set 6 : Changing to Profile_any #Patch Set 7 : clang fromatting #
Total comments: 5
Patch Set 8 : Rebase #Messages
Total messages: 78 (0 generated)
PTAL. I have made the changes as noted in the bug. https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... File media/base/assert_matching_enums.cc (right): https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... media/base/assert_matching_enums.cc:2: // Use of this source code is governed by a BSD-style license that can be This should avoid anyone accidentally changing entry in only one of the files.
Thanks for taking this! Here are my first round of comments. +fgalligan: Will we have any profile for VP9? https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/cont... File content/renderer/pepper/content_decryptor_delegate.cc (right): https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/cont... content/renderer/pepper/content_decryptor_delegate.cc:165: // media::VideoCodecProfile and remove these two cases. This TODO can be removed now. https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_decoder_host.cc:56: case PP_VIDEOPROFILE_VP8MAIN: Also rename the PP enums? https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... File media/base/assert_matching_enums.cc (right): https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... media/base/assert_matching_enums.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2014/07/30 10:22:41, amogh.bihani wrote: > This should avoid anyone accidentally changing entry in only one of the files. I think we are doing the conversion explicitly in PPToMediaProfile() so the media values and PP values doesn't need to be the same: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:64: VP9PROFILE_MIN = 12, We don't need VP8PROFILE_MAIN, VP8PROFILE_MAX and VP8PROFILE_MIN anymore. Can we drop them? https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:67: VP9PROFILE_MAX = VP9PROFILE_MAIN, ditto
Technically VP8 does have profiles (see the spec at http://www.rfc-editor.org/rfc/rfc6386.txt), but we don't really distinguish between then, that is true. Personally I feel NOT_NEEDED is confusing as well though. How about VP8_PROFILE_UNSPECIFIED?
On 2014/07/31 03:37:46, posciak1 wrote: > Technically VP8 does have profiles (see the spec at > http://www.rfc-editor.org/rfc/rfc6386.txt), Thanks for the info :) >but we don't really distinguish between then, that is true. > > Personally I feel NOT_NEEDED is confusing as well though. How about > VP8_PROFILE_UNSPECIFIED? If I have understood it correctly then the header has a 3 bit info of the profile... right? Then we cannot say UNSPECIFIED. If we do not distinguish then we should have something like NOT_CONSIDERED or NOT_USED(but that brings us back to NOT_NEEDED :/ ) Please help me here.
On 2014/07/31 04:05:54, amogh.bihani wrote: > On 2014/07/31 03:37:46, posciak1 wrote: > > Technically VP8 does have profiles (see the spec at > > http://www.rfc-editor.org/rfc/rfc6386.txt), > Thanks for the info :) > > >but we don't really distinguish between then, that is true. > > > > Personally I feel NOT_NEEDED is confusing as well though. How about > > VP8_PROFILE_UNSPECIFIED? > If I have understood it correctly then the header has a 3 bit info of the > profile... right? > Then we cannot say UNSPECIFIED. > If we do not distinguish then we should have something like NOT_CONSIDERED or > NOT_USED(but that brings us back to NOT_NEEDED :/ ) > > Please help me here. That's what I meant by UNSPECIFIED. We are not specifying it, i.e. we don't care, or don't know, or both.
+r: teravest for ppapi and pepper. https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/418193003/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_decoder_host.cc:56: case PP_VIDEOPROFILE_VP8MAIN: On 2014/07/30 21:21:41, xhwang wrote: > Also rename the PP enums? Done. https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... File media/base/assert_matching_enums.cc (right): https://codereview.chromium.org/418193003/diff/1/media/base/assert_matching_e... media/base/assert_matching_enums.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2014/07/30 21:21:41, xhwang wrote: > On 2014/07/30 10:22:41, amogh.bihani wrote: > > This should avoid anyone accidentally changing entry in only one of the files. > > I think we are doing the conversion explicitly in PPToMediaProfile() so the > media values and PP values doesn't need to be the same: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... Acknowledged. I'll remove this :). https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:64: VP9PROFILE_MIN = 12, On 2014/07/30 21:21:41, xhwang wrote: > We don't need VP8PROFILE_MAIN, VP8PROFILE_MAX and VP8PROFILE_MIN anymore. Can we > drop them? Done. https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:67: VP9PROFILE_MAX = VP9PROFILE_MAIN, On 2014/07/30 21:21:41, xhwang wrote: > ditto Done.
+dmichael to confirm renaming enum members used by a dev channel API is okay. https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:21: PP_VIDEOPROFILE_VP8PROFILE_UNSPECIFIED = 11, Since these are only used by a dev channel API (PPB_VideoDecoder), I think renaming these enum members is probably okay. However, I'd like dmichael to confirm that's okay with him.
https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:64: VP9PROFILE_MIN = 12, On 2014/07/31 05:07:40, amogh.bihani wrote: > On 2014/07/30 21:21:41, xhwang wrote: > > We don't need VP8PROFILE_MAIN, VP8PROFILE_MAX and VP8PROFILE_MIN anymore. Can > we > > drop them? > > Done. Can we drop VP8PROFILE_MIN and VP8PROFILE_MAX also? https://codereview.chromium.org/418193003/diff/1/media/base/video_decoder_con... media/base/video_decoder_config.h:67: VP9PROFILE_MAX = VP9PROFILE_MAIN, On 2014/07/31 05:07:40, amogh.bihani wrote: > On 2014/07/30 21:21:41, xhwang wrote: > > ditto > > Done. ditto https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... content/common/gpu/media/android_video_decode_accelerator.cc:92: if (profile == media::VP8PROFILE_UNSPECIFIED) { I think here you mean for "vp8", "profile" is "unspecified" in the spec. But when I look at this, "unspecified" means that it should be specified but not specified, which isn't the case here. So "unspecified" here is ambiguous. I think I still like "NOT_NEEDED" better. Pawel, WDYT? Another option is to use "NOT_APPLICABLE", as in N/A: http://en.wikipedia.org/wiki/N/a Based on wikipedia, the meaning is exact what Pawel mentioned in his comment. Also, we only need one "NOT_NEEDED", which can be shared by all codecs that don't need a profile (e.g. vp8/vp9). So how about just having: media::PROFILE_NOT_NEEDED, or media::PROFILE_NOT_APPLICABLE
-teravest and dmichael, +bbudge for pepper https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:21: PP_VIDEOPROFILE_VP8PROFILE_UNSPECIFIED = 11, On 2014/07/31 15:35:28, teravest wrote: > Since these are only used by a dev channel API (PPB_VideoDecoder), I think > renaming these enum members is probably okay. However, I'd like dmichael to > confirm that's okay with him. Totally fine here. Wouldn't even break backwards-compat (which you guys don't use) since the integer value is the same.
Pepper changes LGTM although clients requesting VP8 or VP9 will have to edit to fix their compile. Since it's not a stable API and pretty new, I think it's OK. Doesn't VP9 have profiles though? This article seems to imply it does: http://en.wikipedia.org/wiki/VP9
On 2014/07/31 17:51:49, bbudge wrote: > Pepper changes LGTM although clients requesting VP8 or VP9 will have to edit to > fix their compile. Since it's not a stable API and pretty new, I think it's OK. > > Doesn't VP9 have profiles though? This article seems to imply it does: > > http://en.wikipedia.org/wiki/VP9 As I understand it, the VP9 profile will be part of the codec name, e.g. vp9.0 https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... +fgalligan to confirm that.
https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/418193003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:21: PP_VIDEOPROFILE_VP8PROFILE_UNSPECIFIED = 11, Naming suggestion - this is a little redundant. How about PP_VIDEOPROFILE_VP8_UNSPECIFIED ?
On 2014/07/31 17:54:29, xhwang wrote: > On 2014/07/31 17:51:49, bbudge wrote: > > Pepper changes LGTM although clients requesting VP8 or VP9 will have to edit > to > > fix their compile. Since it's not a stable API and pretty new, I think it's > OK. > > > > Doesn't VP9 have profiles though? This article seems to imply it does: > > > > http://en.wikipedia.org/wiki/VP9 > > As I understand it, the VP9 profile will be part of the codec name, e.g. vp9.0 > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util... > > +fgalligan to confirm that. That is what I was advocating, but not really up to me (I think). +jimbankoski and +yaowu to talk about how to handle the new features of vp9.
xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern regarding the other suggesion, could you please clarify on that. https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... content/common/gpu/media/android_video_decode_accelerator.cc:92: if (profile == media::VP8PROFILE_UNSPECIFIED) { On 2014/07/31 17:12:44, xhwang wrote: > I think here you mean for "vp8", "profile" is "unspecified" in the spec. But > when I look at this, "unspecified" means that it should be specified but not > specified, which isn't the case here. So "unspecified" here is ambiguous. > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > Another option is to use "NOT_APPLICABLE", as in N/A: > http://en.wikipedia.org/wiki/N/a > Na has a wiki page! :o > Based on wikipedia, the meaning is exact what Pawel mentioned in his comment. > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs that > don't need a profile (e.g. vp8/vp9). So how about just having: > media::PROFILE_NOT_NEEDED, or > media::PROFILE_NOT_APPLICABLE Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose the distinction that we have between these two codecs, which might be used perhaps later.
On 2014/08/01 05:16:24, amogh.bihani wrote: > xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern regarding > the other suggesion, could you please clarify on that. > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > content/common/gpu/media/android_video_decode_accelerator.cc:92: if (profile == > media::VP8PROFILE_UNSPECIFIED) { > On 2014/07/31 17:12:44, xhwang wrote: > > I think here you mean for "vp8", "profile" is "unspecified" in the spec. But > > when I look at this, "unspecified" means that it should be specified but not > > specified, which isn't the case here. So "unspecified" here is ambiguous. > > > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > > > Another option is to use "NOT_APPLICABLE", as in N/A: > > http://en.wikipedia.org/wiki/N/a > > > Na has a wiki page! :o > > Based on wikipedia, the meaning is exact what Pawel mentioned in his comment. > > > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs that > > don't need a profile (e.g. vp8/vp9). So how about just having: > > media::PROFILE_NOT_NEEDED, or > > media::PROFILE_NOT_APPLICABLE > > Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose the > distinction that we have between these two codecs, which might be used perhaps > later. If a codec doesn't use profiles, then having a separate PROFILE_NOT_APPLICABLE won't help distinguish codecs anyways. WDYT?
On 2014/08/01 23:20:01, xhwang wrote: > On 2014/08/01 05:16:24, amogh.bihani wrote: > > xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern regarding > > the other suggesion, could you please clarify on that. > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > content/common/gpu/media/android_video_decode_accelerator.cc:92: if (profile > == > > media::VP8PROFILE_UNSPECIFIED) { > > On 2014/07/31 17:12:44, xhwang wrote: > > > I think here you mean for "vp8", "profile" is "unspecified" in the spec. But > > > when I look at this, "unspecified" means that it should be specified but not > > > specified, which isn't the case here. So "unspecified" here is ambiguous. > > > > > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > > > > > Another option is to use "NOT_APPLICABLE", as in N/A: > > > http://en.wikipedia.org/wiki/N/a > > > > > Na has a wiki page! :o > > > Based on wikipedia, the meaning is exact what Pawel mentioned in his > comment. > > > > > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs that > > > don't need a profile (e.g. vp8/vp9). So how about just having: > > > media::PROFILE_NOT_NEEDED, or > > > media::PROFILE_NOT_APPLICABLE > > > > Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose the > > distinction that we have between these two codecs, which might be used perhaps > > later. > > If a codec doesn't use profiles, then having a separate PROFILE_NOT_APPLICABLE > won't help distinguish codecs anyways. WDYT? I agree that it is intutive to have single enum entry for both VP8 and 9. But the situation gets tricky over here https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... If we have something like media::VIDEO_CODEC_PROFILE_NOT_APPLICABLE, then we cannot decisively say type = kVideoCodecVP8 or 9. :/ Though we currently only use VP8 but what if we want to extend support to VP9, then again we will have to add these entries.
On 2014/08/04 04:41:31, amogh.bihani wrote: > On 2014/08/01 23:20:01, xhwang wrote: > > On 2014/08/01 05:16:24, amogh.bihani wrote: > > > xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern > regarding > > > the other suggesion, could you please clarify on that. > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > content/common/gpu/media/android_video_decode_accelerator.cc:92: if (profile > > == > > > media::VP8PROFILE_UNSPECIFIED) { > > > On 2014/07/31 17:12:44, xhwang wrote: > > > > I think here you mean for "vp8", "profile" is "unspecified" in the spec. > But > > > > when I look at this, "unspecified" means that it should be specified but > not > > > > specified, which isn't the case here. So "unspecified" here is ambiguous. > > > > > > > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > > > > > > > Another option is to use "NOT_APPLICABLE", as in N/A: > > > > http://en.wikipedia.org/wiki/N/a > > > > > > > Na has a wiki page! :o > > > > Based on wikipedia, the meaning is exact what Pawel mentioned in his > > comment. > > > > > > > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs > that > > > > don't need a profile (e.g. vp8/vp9). So how about just having: > > > > media::PROFILE_NOT_NEEDED, or > > > > media::PROFILE_NOT_APPLICABLE > > > > > > Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose the > > > distinction that we have between these two codecs, which might be used > perhaps > > > later. > > > > If a codec doesn't use profiles, then having a separate PROFILE_NOT_APPLICABLE > > won't help distinguish codecs anyways. WDYT? > > I agree that it is intutive to have single enum entry for both VP8 and 9. But > the situation gets tricky over here > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > If we have something like media::VIDEO_CODEC_PROFILE_NOT_APPLICABLE, then we > cannot decisively say type = kVideoCodecVP8 or 9. :/ > Though we currently only use VP8 but what if we want to extend support to VP9, > then again we will have to add these entries. I agree with this. Currently we use profiles to distinguish between codecs as well. Unless we modify quite a few APIs (including VideoDecodeAccelerator and VideoEncodeAccelerator), to include both codec name and profile, we can't merge them...
On 2014/08/04 04:58:00, Pawel Osciak wrote: > On 2014/08/04 04:41:31, amogh.bihani wrote: > > On 2014/08/01 23:20:01, xhwang wrote: > > > On 2014/08/01 05:16:24, amogh.bihani wrote: > > > > xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern > > regarding > > > > the other suggesion, could you please clarify on that. > > > > > > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > > content/common/gpu/media/android_video_decode_accelerator.cc:92: if > (profile > > > == > > > > media::VP8PROFILE_UNSPECIFIED) { > > > > On 2014/07/31 17:12:44, xhwang wrote: > > > > > I think here you mean for "vp8", "profile" is "unspecified" in the spec. > > But > > > > > when I look at this, "unspecified" means that it should be specified but > > not > > > > > specified, which isn't the case here. So "unspecified" here is > ambiguous. > > > > > > > > > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > > > > > > > > > Another option is to use "NOT_APPLICABLE", as in N/A: > > > > > http://en.wikipedia.org/wiki/N/a > > > > > > > > > Na has a wiki page! :o > > > > > Based on wikipedia, the meaning is exact what Pawel mentioned in his > > > comment. > > > > > > > > > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs > > that > > > > > don't need a profile (e.g. vp8/vp9). So how about just having: > > > > > media::PROFILE_NOT_NEEDED, or > > > > > media::PROFILE_NOT_APPLICABLE > > > > > > > > Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose > the > > > > distinction that we have between these two codecs, which might be used > > perhaps > > > > later. > > > > > > If a codec doesn't use profiles, then having a separate > PROFILE_NOT_APPLICABLE > > > won't help distinguish codecs anyways. WDYT? > > > > I agree that it is intutive to have single enum entry for both VP8 and 9. But > > the situation gets tricky over here > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > If we have something like media::VIDEO_CODEC_PROFILE_NOT_APPLICABLE, then we > > cannot decisively say type = kVideoCodecVP8 or 9. :/ > > Though we currently only use VP8 but what if we want to extend support to VP9, > > then again we will have to add these entries. > > I agree with this. Currently we use profiles to distinguish between codecs as > well. Unless we modify quite a few APIs (including VideoDecodeAccelerator and > VideoEncodeAccelerator), to include both codec name and profile, we can't merge > them... This is also the reason why I'm not particularly happy with the name "NOT_APPLICABLE", because those APIs use profiles to specify codec and to query if the particular implementation supports it. Asking for NOT_APPLICABLE doesn't make sense here.
On 2014/08/04 05:00:23, Pawel Osciak wrote: > On 2014/08/04 04:58:00, Pawel Osciak wrote: > > On 2014/08/04 04:41:31, amogh.bihani wrote: > > > On 2014/08/01 23:20:01, xhwang wrote: > > > > On 2014/08/01 05:16:24, amogh.bihani wrote: > > > > > xhwang@: I have changed the names to NOT_APPLICABLE, I had a concern > > > regarding > > > > > the other suggesion, could you please clarify on that. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > > > File content/common/gpu/media/android_video_decode_accelerator.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/418193003/diff/60001/content/common/gpu/media... > > > > > content/common/gpu/media/android_video_decode_accelerator.cc:92: if > > (profile > > > > == > > > > > media::VP8PROFILE_UNSPECIFIED) { > > > > > On 2014/07/31 17:12:44, xhwang wrote: > > > > > > I think here you mean for "vp8", "profile" is "unspecified" in the > spec. > > > But > > > > > > when I look at this, "unspecified" means that it should be specified > but > > > not > > > > > > specified, which isn't the case here. So "unspecified" here is > > ambiguous. > > > > > > > > > > > > I think I still like "NOT_NEEDED" better. Pawel, WDYT? > > > > > > > > > > > > Another option is to use "NOT_APPLICABLE", as in N/A: > > > > > > http://en.wikipedia.org/wiki/N/a > > > > > > > > > > > Na has a wiki page! :o > > > > > > Based on wikipedia, the meaning is exact what Pawel mentioned in his > > > > comment. > > > > > > > > > > > > Also, we only need one "NOT_NEEDED", which can be shared by all codecs > > > that > > > > > > don't need a profile (e.g. vp8/vp9). So how about just having: > > > > > > media::PROFILE_NOT_NEEDED, or > > > > > > media::PROFILE_NOT_APPLICABLE > > > > > > > > > > Won't creating single NOT_APPLICABLE profile for vp8/9 will make us lose > > the > > > > > distinction that we have between these two codecs, which might be used > > > perhaps > > > > > later. > > > > > > > > If a codec doesn't use profiles, then having a separate > > PROFILE_NOT_APPLICABLE > > > > won't help distinguish codecs anyways. WDYT? > > > > > > I agree that it is intutive to have single enum entry for both VP8 and 9. > But > > > the situation gets tricky over here > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > If we have something like media::VIDEO_CODEC_PROFILE_NOT_APPLICABLE, then we > > > cannot decisively say type = kVideoCodecVP8 or 9. :/ > > > Though we currently only use VP8 but what if we want to extend support to > VP9, > > > then again we will have to add these entries. > > > > I agree with this. Currently we use profiles to distinguish between codecs as > > well. Unless we modify quite a few APIs (including VideoDecodeAccelerator and > > VideoEncodeAccelerator), to include both codec name and profile, we can't > merge > > them... > > This is also the reason why I'm not particularly happy with the name > "NOT_APPLICABLE", because those APIs use profiles to specify codec and to query > if the particular implementation supports it. Asking for NOT_APPLICABLE doesn't > make sense here. Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use case it actually means codec + profile. It seems VEA always accepts profiles from MIN to MAX. So it doesn't really care about profiles, it only needs to know what codec is used. Is this correct? If so, why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we do care, we should rename it to VEA::SupportedCodecAndProfile. That being said, I agree this is out of the scope of this CL. So having two N/A entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE still sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO.
> Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use > case > it actually means codec + profile. > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really care > about profiles, it only needs to know what codec is used. Is this correct? If > so, > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we do > care, we should rename it to VEA::SupportedCodecAndProfile. > > That being said, I agree this is out of the scope of this CL. So having two N/A > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE still > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. Now I am getting inclined towards NOT_NEEDED because even if the profile is present/specified, browser does not _need_ to know the actual profile. Pawel WDYT?
On 2014/08/04 16:48:06, xhwang wrote: > On 2014/08/04 05:00:23, Pawel Osciak wrote: > > > I agree with this. Currently we use profiles to distinguish between codecs > as > > > well. Unless we modify quite a few APIs (including VideoDecodeAccelerator > and > > > VideoEncodeAccelerator), to include both codec name and profile, we can't > > merge > > > them... > > > > This is also the reason why I'm not particularly happy with the name > > "NOT_APPLICABLE", because those APIs use profiles to specify codec and to > query > > if the particular implementation supports it. Asking for NOT_APPLICABLE > doesn't > > make sense here. > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use > case > it actually means codec + profile. > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really care > about profiles, it only needs to know what codec is used. Is this correct? If > so, > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we do > care, we should rename it to VEA::SupportedCodecAndProfile. > That being said, I agree this is out of the scope of this CL. So having two N/A > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE still > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. VDA still needs the exact profile though. It takes profile on VDA::Initialize, it has to be provided by the client, and VDA makes the decision whether it can decode it or not in there. So it's not really not "NOT_APPLICABLE" or "NOT_NEEDED" ;)
On 2014/08/05 02:55:45, amogh.bihani wrote: > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use > > case > > it actually means codec + profile. > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really > care > > about profiles, it only needs to know what codec is used. Is this correct? If > > so, > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we > do > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > That being said, I agree this is out of the scope of this CL. So having two > N/A > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE still > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > Now I am getting inclined towards NOT_NEEDED because even if the profile is > present/specified, browser does not _need_ to know the actual profile. > Pawel WDYT? VDA::Initialize needs the exact profile (for H264). As for VP8/9, it doesn't, but it still needs VP8 vs VP9.
On 2014/08/05 04:08:22, Pawel Osciak wrote: > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use > > > case > > > it actually means codec + profile. > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really > > care > > > about profiles, it only needs to know what codec is used. Is this correct? > If > > > so, > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we > > do > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > That being said, I agree this is out of the scope of this CL. So having two > > N/A > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE > still > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile is > > present/specified, browser does not _need_ to know the actual profile. > > Pawel WDYT? > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it doesn't, > but it still needs VP8 vs VP9. This CL doesn't change how we specify profile for H.264. For VP8/VP9, VP8_PROFILE_NOT_NEEDED and VP9_PROFILE_NOT_NEEDED should suffice what VDA needs. Will that be okay?
On 2014/08/05 04:08:22, Pawel Osciak wrote: > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above use > > > case > > > it actually means codec + profile. > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really > > care > > > about profiles, it only needs to know what codec is used. Is this correct? > If > > > so, > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if we > > do > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > That being said, I agree this is out of the scope of this CL. So having two > > N/A > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE > still > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile is > > present/specified, browser does not _need_ to know the actual profile. > > Pawel WDYT? > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it doesn't, > but it still needs VP8 vs VP9. so VP8EXACT_PROFILE_NOT_NEEDED instead of VP8PROFILE_NOT_NEEDED?
On 2014/08/05 04:14:18, amogh.bihani wrote: > On 2014/08/05 04:08:22, Pawel Osciak wrote: > > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above > use > > > > case > > > > it actually means codec + profile. > > > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really > > > care > > > > about profiles, it only needs to know what codec is used. Is this correct? > > If > > > > so, > > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if > we > > > do > > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > > > That being said, I agree this is out of the scope of this CL. So having > two > > > N/A > > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE > > still > > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile is > > > present/specified, browser does not _need_ to know the actual profile. > > > Pawel WDYT? > > > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it doesn't, > > but it still needs VP8 vs VP9. > > so VP8EXACT_PROFILE_NOT_NEEDED instead of VP8PROFILE_NOT_NEEDED? Oh sorry didn't see xhwang's comments.
On 2014/08/05 04:12:11, xhwang wrote: > On 2014/08/05 04:08:22, Pawel Osciak wrote: > > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above > use > > > > case > > > > it actually means codec + profile. > > > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't really > > > care > > > > about profiles, it only needs to know what codec is used. Is this correct? > > If > > > > so, > > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even if > we > > > do > > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > > > That being said, I agree this is out of the scope of this CL. So having > two > > > N/A > > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE > > still > > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile is > > > present/specified, browser does not _need_ to know the actual profile. > > > Pawel WDYT? > > > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it doesn't, > > but it still needs VP8 vs VP9. > > This CL doesn't change how we specify profile for H.264. For VP8/VP9, > VP8_PROFILE_NOT_NEEDED and VP9_PROFILE_NOT_NEEDED should suffice what > VDA needs. Will that be okay? Yes. Last try though: if people really don't like UNSPECIFIED, how about "VPX_PROFILE_ANY"? :)
On 2014/08/05 04:15:42, Pawel Osciak wrote: > On 2014/08/05 04:12:11, xhwang wrote: > > On 2014/08/05 04:08:22, Pawel Osciak wrote: > > > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the above > > use > > > > > case > > > > > it actually means codec + profile. > > > > > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't > really > > > > care > > > > > about profiles, it only needs to know what codec is used. Is this > correct? > > > If > > > > > so, > > > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even > if > > we > > > > do > > > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > > > > > That being said, I agree this is out of the scope of this CL. So having > > two > > > > N/A > > > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and NOT_APPLICABLE > > > still > > > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear IMHO. > > > > > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile > is > > > > present/specified, browser does not _need_ to know the actual profile. > > > > Pawel WDYT? > > > > > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it > doesn't, > > > but it still needs VP8 vs VP9. > > > > This CL doesn't change how we specify profile for H.264. For VP8/VP9, > > VP8_PROFILE_NOT_NEEDED and VP9_PROFILE_NOT_NEEDED should suffice what > > VDA needs. Will that be okay? > > Yes. > Last try though: if people really don't like UNSPECIFIED, how about > "VPX_PROFILE_ANY"? :) ANY means anything. In our case, it's really NONE...
On 2014/08/05 04:18:13, xhwang wrote: > On 2014/08/05 04:15:42, Pawel Osciak wrote: > > On 2014/08/05 04:12:11, xhwang wrote: > > > On 2014/08/05 04:08:22, Pawel Osciak wrote: > > > > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the > above > > > use > > > > > > case > > > > > > it actually means codec + profile. > > > > > > > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't > > really > > > > > care > > > > > > about profiles, it only needs to know what codec is used. Is this > > correct? > > > > If > > > > > > so, > > > > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? Even > > if > > > we > > > > > do > > > > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > > > > > > > That being said, I agree this is out of the scope of this CL. So > having > > > two > > > > > N/A > > > > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and > NOT_APPLICABLE > > > > still > > > > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear > IMHO. > > > > > > > > > > Now I am getting inclined towards NOT_NEEDED because even if the profile > > is > > > > > present/specified, browser does not _need_ to know the actual profile. > > > > > Pawel WDYT? > > > > > > > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it > > doesn't, > > > > but it still needs VP8 vs VP9. > > > > > > This CL doesn't change how we specify profile for H.264. For VP8/VP9, > > > VP8_PROFILE_NOT_NEEDED and VP9_PROFILE_NOT_NEEDED should suffice what > > > VDA needs. Will that be okay? > > > > Yes. > > Last try though: if people really don't like UNSPECIFIED, how about > > "VPX_PROFILE_ANY"? :) > > ANY means anything. In our case, it's really NONE... So are we back to Patch set #1? :p (VP8PROFILE_NOT_NEEDED & VP9PROFILE_NOT_NEEDED)
On 2014/08/05 04:54:48, amogh.bihani wrote: > On 2014/08/05 04:18:13, xhwang wrote: > > On 2014/08/05 04:15:42, Pawel Osciak wrote: > > > On 2014/08/05 04:12:11, xhwang wrote: > > > > On 2014/08/05 04:08:22, Pawel Osciak wrote: > > > > > On 2014/08/05 02:55:45, amogh.bihani wrote: > > > > > > > Sigh, the VideoCodecProfile enum gets a bit ambiguous here. In the > > above > > > > use > > > > > > > case > > > > > > > it actually means codec + profile. > > > > > > > > > > > > > > It seems VEA always accepts profiles from MIN to MAX. So it doesn't > > > really > > > > > > care > > > > > > > about profiles, it only needs to know what codec is used. Is this > > > correct? > > > > > If > > > > > > > so, > > > > > > > why don't we change VEA::SupportedProfile to VEA::SupportedCodec? > Even > > > if > > > > we > > > > > > do > > > > > > > care, we should rename it to VEA::SupportedCodecAndProfile. > > > > > > > > > > > > > > That being said, I agree this is out of the scope of this CL. So > > having > > > > two > > > > > > N/A > > > > > > > entries for VP8 and VP9 makes sense to me. NOT_NEEDED and > > NOT_APPLICABLE > > > > > still > > > > > > > sgtm. It's the VEA code abusing the enum; the enum itself is clear > > IMHO. > > > > > > > > > > > > Now I am getting inclined towards NOT_NEEDED because even if the > profile > > > is > > > > > > present/specified, browser does not _need_ to know the actual profile. > > > > > > Pawel WDYT? > > > > > > > > > > VDA::Initialize needs the exact profile (for H264). As for VP8/9, it > > > doesn't, > > > > > but it still needs VP8 vs VP9. > > > > > > > > This CL doesn't change how we specify profile for H.264. For VP8/VP9, > > > > VP8_PROFILE_NOT_NEEDED and VP9_PROFILE_NOT_NEEDED should suffice what > > > > VDA needs. Will that be okay? > > > > > > Yes. > > > Last try though: if people really don't like UNSPECIFIED, how about > > > "VPX_PROFILE_ANY"? :) > > > > ANY means anything. In our case, it's really NONE... > > So are we back to Patch set #1? :p > (VP8PROFILE_NOT_NEEDED & VP9PROFILE_NOT_NEEDED) Ping!
Since VP8 and VP9 have the concept of profiles, but Chrome doesn't require (or allow) a profile to be specified, I like VP8PROFILE_ANY, since it expresses that the code will work with any profile as long as it's VP8. If we add the ability to specify a profile later, we can add enum values and this value will still make sense. This is important mostly for Pepper, where we can't change the API later. Another possibility is VP8PROFILE_ALL but it isn't as future-proof.
On 2014/08/06 15:51:39, bbudge wrote: > Since VP8 and VP9 have the concept of profiles, but Chrome doesn't require (or > allow) a profile to be specified, I like VP8PROFILE_ANY, since it expresses that > the code will work with any profile as long as it's VP8. If we add the ability > to specify a profile later, we can add enum values and this value will still > make sense. This is important mostly for Pepper, where we can't change the API > later. > > Another possibility is VP8PROFILE_ALL but it isn't as future-proof. ANY sgtm. Let's go with ANY then.
On 2014/08/06 16:04:25, xhwang wrote: > On 2014/08/06 15:51:39, bbudge wrote: > > Since VP8 and VP9 have the concept of profiles, but Chrome doesn't require (or > > allow) a profile to be specified, I like VP8PROFILE_ANY, since it expresses > that > > the code will work with any profile as long as it's VP8. If we add the ability > > to specify a profile later, we can add enum values and this value will still > > make sense. This is important mostly for Pepper, where we can't change the API > > later. > > > > Another possibility is VP8PROFILE_ALL but it isn't as future-proof. > > ANY sgtm. Let's go with ANY then. I have made the changes to PROFILE_ANY. PTAL.
content/common/gpu/media lgtm
looking good except for one question https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... media/base/video_decoder_config.h:65: VP9PROFILE_MAX = VP9PROFILE_ANY, hmm, maybe I forgot... why can't we just drop VP*PROFILE_MIN and VP*PROFILE_MAX? https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... File ppapi/api/dev/pp_video_dev.idl (right): https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... ppapi/api/dev/pp_video_dev.idl:34: PP_VIDEODECODER_VP8PROFILE_ANY = 12 For the future, shall we add VP9_PROFILE_ANY?
https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... media/base/video_decoder_config.h:65: VP9PROFILE_MAX = VP9PROFILE_ANY, On 2014/08/07 05:56:24, xhwang wrote: > hmm, maybe I forgot... why can't we just drop VP*PROFILE_MIN and VP*PROFILE_MAX? VDA and VEA use profile to get/indicate the codec from/to their respective clients.
On 2014/08/07 06:00:31, Pawel Osciak wrote: > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > File media/base/video_decoder_config.h (right): > > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > media/base/video_decoder_config.h:65: VP9PROFILE_MAX = VP9PROFILE_ANY, > On 2014/08/07 05:56:24, xhwang wrote: > > hmm, maybe I forgot... why can't we just drop VP*PROFILE_MIN and > VP*PROFILE_MAX? > > VDA and VEA use profile to get/indicate the codec from/to their respective > clients. Sorry for not being clear. I meant why can't we drop VP8PROFILE_MIN? Can't VDA and VEA just check VP8PROFILE_ANY? The same applies to VP9*.
On 2014/08/07 06:08:30, xhwang wrote: > On 2014/08/07 06:00:31, Pawel Osciak wrote: > > > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > > File media/base/video_decoder_config.h (right): > > > > > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > > media/base/video_decoder_config.h:65: VP9PROFILE_MAX = VP9PROFILE_ANY, > > On 2014/08/07 05:56:24, xhwang wrote: > > > hmm, maybe I forgot... why can't we just drop VP*PROFILE_MIN and > > VP*PROFILE_MAX? > > > > VDA and VEA use profile to get/indicate the codec from/to their respective > > clients. > > Sorry for not being clear. I meant why can't we drop VP8PROFILE_MIN? Can't VDA > and VEA just check VP8PROFILE_ANY? The same applies to VP9*. Ah I misunderstood you. I'd personally prefer to keep them, just for symmetry for MIN<x<MAX checks. There are the same value anyway.
https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... File ppapi/api/dev/pp_video_dev.idl (right): https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... ppapi/api/dev/pp_video_dev.idl:34: PP_VIDEODECODER_VP8PROFILE_ANY = 12 On 2014/08/07 05:56:24, xhwang wrote: > For the future, shall we add VP9_PROFILE_ANY? I thought about it but I think we should add it when we also implement it, on similar lines to what is said for codecs here https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_d...
On 2014/08/07 06:08:30, xhwang wrote: > On 2014/08/07 06:00:31, Pawel Osciak wrote: > > > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > > File media/base/video_decoder_config.h (right): > > > > > https://codereview.chromium.org/418193003/diff/120001/media/base/video_decode... > > media/base/video_decoder_config.h:65: VP9PROFILE_MAX = VP9PROFILE_ANY, > > On 2014/08/07 05:56:24, xhwang wrote: > > > hmm, maybe I forgot... why can't we just drop VP*PROFILE_MIN and > > VP*PROFILE_MAX? > > > > VDA and VEA use profile to get/indicate the codec from/to their respective > > clients. > > Sorry for not being clear. I meant why can't we drop VP8PROFILE_MIN? Can't VDA > and VEA just check VP8PROFILE_ANY? The same applies to VP9*. Hmmm... if we are sure that profiles won't be used in the near future, then we can remove it. I would like to know if everyone is ok with this.
https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... File ppapi/api/dev/pp_video_dev.idl (right): https://codereview.chromium.org/418193003/diff/120001/ppapi/api/dev/pp_video_... ppapi/api/dev/pp_video_dev.idl:34: PP_VIDEODECODER_VP8PROFILE_ANY = 12 Adding an enum value would probably require versioning the API, which is a pain. We should hold off on that, as this API is slated to be replaced by a new PPB_VIdeoDecoder interface (see pp_codecs.idl.)
The CL itself looks good and we can clean up it more if needed in the future. Thanks for taking this and sorry for the lengthy discussion. lgtm
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/418193003/120001
The CQ bit was unchecked by amogh.bihani@samsung.com
+r: noelallen@ for native_client_sdk. PTAL.
On 2014/08/08 03:09:17, amogh.bihani wrote: > +r: noelallen@ for native_client_sdk. > > PTAL. Noelallen@. PTAL
On 2014/08/12 01:05:40, amogh.bihani wrote: > On 2014/08/08 03:09:17, amogh.bihani wrote: > > +r: noelallen@ for native_client_sdk. > > > > PTAL. > > Noelallen@. PTAL Using TBR for now. noelallen@ please have a look later and let me know if anything else is needed. :) Thanks
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/418193003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Presubmit is giving this warning PPAPI Interface modified without updating NaCl SDK. (note that some dev interfaces should not be added the NaCl SDK; when in doubt, ask a ppapi OWNER.) In native_client_sdk/src/libraries/ppapi/library.dsc: these files are missing and should be added: ppapi/c/dev/pp_video_dev.h Do I need to add the file there?
On 2014/08/13 09:08:41, amogh.bihani wrote: > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Presubmit is giving this warning > > PPAPI Interface modified without updating NaCl SDK. > (note that some dev interfaces should not be added the NaCl SDK; when in doubt, > ask a ppapi OWNER.) > > In native_client_sdk/src/libraries/ppapi/library.dsc: > these files are missing and should be added: > ppapi/c/dev/pp_video_dev.h > > > Do I need to add the file there? I don't think so. This API is only used by Flash.
On 2014/08/13 11:21:46, bbudge wrote: > On 2014/08/13 09:08:41, amogh.bihani wrote: > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on tryserver.chromium.linux > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > Presubmit is giving this warning > > > > PPAPI Interface modified without updating NaCl SDK. > > (note that some dev interfaces should not be added the NaCl SDK; when in > doubt, > > ask a ppapi OWNER.) > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > these files are missing and should be added: > > ppapi/c/dev/pp_video_dev.h > > > > > > Do I need to add the file there? > > I don't think so. This API is only used by Flash. Umm... so how do we make presubmit script happy?
On 2014/08/13 11:23:34, amogh.bihani wrote: > On 2014/08/13 11:21:46, bbudge wrote: > > On 2014/08/13 09:08:41, amogh.bihani wrote: > > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > > Try jobs failed on following builders: > > > > chromium_presubmit on tryserver.chromium.linux > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > Presubmit is giving this warning > > > > > > PPAPI Interface modified without updating NaCl SDK. > > > (note that some dev interfaces should not be added the NaCl SDK; when in > > doubt, > > > ask a ppapi OWNER.) > > > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > > these files are missing and should be added: > > > ppapi/c/dev/pp_video_dev.h > > > > > > > > > Do I need to add the file there? > > > > I don't think so. This API is only used by Flash. > > Umm... so how do we make presubmit script happy? All I can think of is to commit manually rather than use CQ.
On 2014/08/13 11:28:23, bbudge wrote: > On 2014/08/13 11:23:34, amogh.bihani wrote: > > On 2014/08/13 11:21:46, bbudge wrote: > > > On 2014/08/13 09:08:41, amogh.bihani wrote: > > > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > > > Try jobs failed on following builders: > > > > > chromium_presubmit on tryserver.chromium.linux > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > > > Presubmit is giving this warning > > > > > > > > PPAPI Interface modified without updating NaCl SDK. > > > > (note that some dev interfaces should not be added the NaCl SDK; when in > > > doubt, > > > > ask a ppapi OWNER.) > > > > > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > > > these files are missing and should be added: > > > > ppapi/c/dev/pp_video_dev.h > > > > > > > > > > > > Do I need to add the file there? > > > > > > I don't think so. This API is only used by Flash. > > > > Umm... so how do we make presubmit script happy? > > All I can think of is to commit manually rather than use CQ. Im not a commiter yet, I don't think I have permission for that. would NOTRY=true work for me?
On 2014/08/13 11:37:29, amogh.bihani wrote: > On 2014/08/13 11:28:23, bbudge wrote: > > On 2014/08/13 11:23:34, amogh.bihani wrote: > > > On 2014/08/13 11:21:46, bbudge wrote: > > > > On 2014/08/13 09:08:41, amogh.bihani wrote: > > > > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > > > > Try jobs failed on following builders: > > > > > > chromium_presubmit on tryserver.chromium.linux > > > > > > > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > > > > > Presubmit is giving this warning > > > > > > > > > > PPAPI Interface modified without updating NaCl SDK. > > > > > (note that some dev interfaces should not be added the NaCl SDK; when in > > > > doubt, > > > > > ask a ppapi OWNER.) > > > > > > > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > > > > these files are missing and should be added: > > > > > ppapi/c/dev/pp_video_dev.h > > > > > > > > > > > > > > > Do I need to add the file there? > > > > > > > > I don't think so. This API is only used by Flash. > > > > > > Umm... so how do we make presubmit script happy? > > > > All I can think of is to commit manually rather than use CQ. > > Im not a commiter yet, I don't think I have permission for that. > would NOTRY=true work for me? I see. I'm not sure if NOTRY or NOTREECHECKS would suppress presubmit checks. Perhaps both? Otherwise, maybe a committer can land it for you.
On 2014/08/13 11:51:06, bbudge wrote: > On 2014/08/13 11:37:29, amogh.bihani wrote: > > On 2014/08/13 11:28:23, bbudge wrote: > > > On 2014/08/13 11:23:34, amogh.bihani wrote: > > > > On 2014/08/13 11:21:46, bbudge wrote: > > > > > On 2014/08/13 09:08:41, amogh.bihani wrote: > > > > > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > > > > > Try jobs failed on following builders: > > > > > > > chromium_presubmit on tryserver.chromium.linux > > > > > > > > > > > > > > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > > > > > > > Presubmit is giving this warning > > > > > > > > > > > > PPAPI Interface modified without updating NaCl SDK. > > > > > > (note that some dev interfaces should not be added the NaCl SDK; when > in > > > > > doubt, > > > > > > ask a ppapi OWNER.) > > > > > > > > > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > > > > > these files are missing and should be added: > > > > > > ppapi/c/dev/pp_video_dev.h > > > > > > > > > > > > > > > > > > Do I need to add the file there? > > > > > > > > > > I don't think so. This API is only used by Flash. > > > > > > > > Umm... so how do we make presubmit script happy? > > > > > > All I can think of is to commit manually rather than use CQ. > > > > Im not a commiter yet, I don't think I have permission for that. > > would NOTRY=true work for me? > > I see. I'm not sure if NOTRY or NOTREECHECKS would suppress presubmit checks. > Perhaps both? > > Otherwise, maybe a committer can land it for you. Sheriff is fixing GN build issue... Will wait till that bot goes green.
On 2014/08/13 12:31:15, amogh.bihani wrote: > On 2014/08/13 11:51:06, bbudge wrote: > > On 2014/08/13 11:37:29, amogh.bihani wrote: > > > On 2014/08/13 11:28:23, bbudge wrote: > > > > On 2014/08/13 11:23:34, amogh.bihani wrote: > > > > > On 2014/08/13 11:21:46, bbudge wrote: > > > > > > On 2014/08/13 09:08:41, amogh.bihani wrote: > > > > > > > On 2014/08/13 08:57:51, I haz the power (commit-bot) wrote: > > > > > > > > Try jobs failed on following builders: > > > > > > > > chromium_presubmit on tryserver.chromium.linux > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > > > > > > > > > Presubmit is giving this warning > > > > > > > > > > > > > > PPAPI Interface modified without updating NaCl SDK. > > > > > > > (note that some dev interfaces should not be added the NaCl SDK; > when > > in > > > > > > doubt, > > > > > > > ask a ppapi OWNER.) > > > > > > > > > > > > > > In native_client_sdk/src/libraries/ppapi/library.dsc: > > > > > > > these files are missing and should be added: > > > > > > > ppapi/c/dev/pp_video_dev.h > > > > > > > > > > > > > > > > > > > > > Do I need to add the file there? > > > > > > > > > > > > I don't think so. This API is only used by Flash. > > > > > > > > > > Umm... so how do we make presubmit script happy? > > > > > > > > All I can think of is to commit manually rather than use CQ. > > > > > > Im not a commiter yet, I don't think I have permission for that. > > > would NOTRY=true work for me? > > > > I see. I'm not sure if NOTRY or NOTREECHECKS would suppress presubmit checks. > > Perhaps both? > > > > Otherwise, maybe a committer can land it for you. > > Sheriff is fixing GN build issue... Will wait till that bot goes green. Trying my luck once with NOTRY and NOTREECHECK. :)
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/418193003/140001
On 2014/08/14 07:02:51, amogh.bihani wrote: > The CQ bit was checked by mailto:amogh.bihani@samsung.com Could we wait for the fix on the GN issue instead of doing NOTRY please?
The CQ bit was unchecked by amogh.bihani@samsung.com
On 2014/08/14 07:05:31, Pawel Osciak wrote: > On 2014/08/14 07:02:51, amogh.bihani wrote: > > The CQ bit was checked by mailto:amogh.bihani@samsung.com > > Could we wait for the fix on the GN issue instead of doing NOTRY please? Ok... I thought the bot went green so it might have been fixed.
On 2014/08/14 07:05:31, Pawel Osciak wrote: > On 2014/08/14 07:02:51, amogh.bihani wrote: > > The CQ bit was checked by mailto:amogh.bihani@samsung.com > > Could we wait for the fix on the GN issue instead of doing NOTRY please? Ok... I thought the bot went green so it might have been fixed.
The CQ bit was unchecked by commit-bot@chromium.org
On 2014/08/14 07:08:27, amogh.bihani wrote: > On 2014/08/14 07:05:31, Pawel Osciak wrote: > > On 2014/08/14 07:02:51, amogh.bihani wrote: > > > The CQ bit was checked by mailto:amogh.bihani@samsung.com > > > > Could we wait for the fix on the GN issue instead of doing NOTRY please? > > Ok... I thought the bot went green so it might have been fixed. Great, please remove NOTRY then. Thanks.
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/418193003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/08/14 08:56:45, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) :( Even that didn't help. Can anyone please submit this manually. I guess we need to change the presubmit behaviour to not to fail at this false positive.
On 2014/08/14 09:12:50, amogh.bihani wrote: > On 2014/08/14 08:56:45, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > :( > Even that didn't help. Can anyone please submit this manually. > I guess we need to change the presubmit behaviour to not to fail at this false > positive. I'll talk to the NaCl SDK folks today to see what needs to be done.
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/418193003/140001
Message was sent while issue was closed.
Committed patchset #8 (140001) as 289765 |
