remoting/client/plugin/pepper_video_renderer_3d.cc - Issue 1769593002: Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Unified Diff: remoting/client/plugin/pepper_video_renderer_3d.cc

Issue 1769593002: Add 4 VP9 profiles to the media::VideoCodecProfile enum. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@hevc-codec-7

Patch Set: Better TODOs Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: remoting/client/plugin/pepper_video_renderer_3d.cc

diff --git a/remoting/client/plugin/pepper_video_renderer_3d.cc b/remoting/client/plugin/pepper_video_renderer_3d.cc

index 4c22b1439c6a824629c577d230bde08aef4a535b..2d89ee3348018b6015b881da4873d1e61550bc69 100644

--- a/remoting/client/plugin/pepper_video_renderer_3d.cc

+++ b/remoting/client/plugin/pepper_video_renderer_3d.cc

@@ -160,7 +160,7 @@ void PepperVideoRenderer3D::OnSessionConfig(

video_profile = PP_VIDEOPROFILE_VP8_ANY;

break;

case protocol::ChannelConfig::CODEC_VP9:

- video_profile = PP_VIDEOPROFILE_VP9_ANY;

+ video_profile = PP_VIDEOPROFILE_VP9_PROFILE3;

DaleCurtis 2016/03/09 18:49:47 Why?

servolk 2016/03/09 19:32:33 Well, previously we used VP9_ANY here, which meant

DaleCurtis 2016/03/09 21:48:47 I think your reasoning is sound, but was calling o

servolk 2016/03/09 21:52:40 Ok, I've made a change to use profile3 in ffmpeg_c

ddorwin 2016/03/09 22:20:05 IIUC, there is not currently a way to specify othe

servolk 2016/03/09 22:27:42 Yes, currently we won't use any other VP9 profiles

Yes, currently we won't use any other VP9 profiles other than those hardcoded in this CL, but hopefully it's going to change soon, as I'm planning to look into reading VP9 profile from the bitstream (or maybe we can just wait until VP9 profile is also provided in codec private data? https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6). And yes, good point about UMA. Currently all profiles get reported as a value that is going to be used for just PROFILE0 after this CL. But I think this is the best we can do, no way to adjust that data retroactively.

ddorwin 2016/03/09 23:30:09 My point is that any data that does not specify th

On 2016/03/09 22:27:42, servolk wrote: > On 2016/03/09 22:20:05, ddorwin wrote: > > On 2016/03/09 21:52:40, servolk wrote: > > > On 2016/03/09 21:48:47, DaleCurtis wrote: > > > > On 2016/03/09 at 19:32:33, servolk wrote: > > > > > On 2016/03/09 18:49:47, DaleCurtis wrote: > > > > > > Why? > > > > > > > > > > Well, previously we used VP9_ANY here, which meant potentially you could > > get > > > > any profile, even the highest profile 3. So to minimize a chance of > > > regressions > > > > I've chosen to use profile3 here (if we used profile0 to initialize the > > > > video_decoder_, then potentially it could refuse to accept higher profiles > > > later > > > > on, but I think if we use profile3 to init the decoder, then using > > > lower-profile > > > > streams later on should be fine). I think I should actually do the same in > > > > media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc, > > > i.e. > > > > use the highest profile, instead of profile0). > > > > > > > > I think your reasoning is sound, but was calling out that this is > > inconsistent > > > > with what you do elsewhere. > > > > > > Ok, I've made a change to use profile3 in ffmpeg_common and > webm_video_client > > in > > > the latest patchset. > > > > IIUC, there is not currently a way to specify other profiles in WebM. Thus, we > > should assume in those other places that it is profile 0. > > > > I don't have any thoughts either way on this file. > > > > Another item to be aware of is that some of these are eventually reported as > > UMAs. If we go with something other than PROFILE0, we'll see step functions in > > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but it's > > something to be aware of. > > Yes, currently we won't use any other VP9 profiles other than those hardcoded in > this CL, but hopefully it's going to change soon, as I'm planning to look into > reading VP9 profile from the bitstream (or maybe we can just wait until VP9 > profile is also provided in codec private data? > https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6). > And yes, good point about UMA. Currently all profiles get reported as a value > that is going to be used for just PROFILE0 after this CL. But I think this is > the best we can do, no way to adjust that data retroactively.

My point is that any data that does not specify the profile in the private data must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until we have such support. Also, all existing content is likely profile 0.

servolk 2016/03/09 23:42:10 I agree that most existing content is likely profi

On 2016/03/09 23:30:09, ddorwin wrote: > On 2016/03/09 22:27:42, servolk wrote: > > On 2016/03/09 22:20:05, ddorwin wrote: > > > On 2016/03/09 21:52:40, servolk wrote: > > > > On 2016/03/09 21:48:47, DaleCurtis wrote: > > > > > On 2016/03/09 at 19:32:33, servolk wrote: > > > > > > On 2016/03/09 18:49:47, DaleCurtis wrote: > > > > > > > Why? > > > > > > > > > > > > Well, previously we used VP9_ANY here, which meant potentially you > could > > > get > > > > > any profile, even the highest profile 3. So to minimize a chance of > > > > regressions > > > > > I've chosen to use profile3 here (if we used profile0 to initialize the > > > > > video_decoder_, then potentially it could refuse to accept higher > profiles > > > > later > > > > > on, but I think if we use profile3 to init the decoder, then using > > > > lower-profile > > > > > streams later on should be fine). I think I should actually do the same > in > > > > > media/ffmpeg/ffmpeg_common.cc and > media/formats/webm/webm_video_client.cc, > > > > i.e. > > > > > use the highest profile, instead of profile0). > > > > > > > > > > I think your reasoning is sound, but was calling out that this is > > > inconsistent > > > > > with what you do elsewhere. > > > > > > > > Ok, I've made a change to use profile3 in ffmpeg_common and > > webm_video_client > > > in > > > > the latest patchset. > > > > > > IIUC, there is not currently a way to specify other profiles in WebM. Thus, > we > > > should assume in those other places that it is profile 0. > > > > > > I don't have any thoughts either way on this file. > > > > > > Another item to be aware of is that some of these are eventually reported as > > > UMAs. If we go with something other than PROFILE0, we'll see step functions > in > > > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but > it's > > > something to be aware of. > > > > Yes, currently we won't use any other VP9 profiles other than those hardcoded > in > > this CL, but hopefully it's going to change soon, as I'm planning to look into > > reading VP9 profile from the bitstream (or maybe we can just wait until VP9 > > profile is also provided in codec private data? > > https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6). > > And yes, good point about UMA. Currently all profiles get reported as a value > > that is going to be used for just PROFILE0 after this CL. But I think this is > > the best we can do, no way to adjust that data retroactively. > > My point is that any data that does not specify the profile in the private data > must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until we > have such support. Also, all existing content is likely profile 0.

I agree that most existing content is likely profile 0. But I'm not sure if I agree that we must assume profile0. All VP9 streams do specify the profile in bitstream header, it's just that currently we don't read it and so we treat any stream as being profile VP9_ANY. That wasn't ideal, but that allowed any profile to work, even profiles 1,2,3. If we used profile0 here, then the video decoder would be initialized with a set of parameters for profile0 and would most likely fail to decode higher profiles. But if we use profile3 here, then the decoder will be ready to decode profile3 and (I'm guessing here) should be also able to decode lower profiles as well, since all lower profiles seem to be subsets of higher profiles. So it should keep things working for all VP9 streams and it shouldn't be any worse than the current situation. The same reasoning applies to the other two places.

servolk 2016/03/24 18:09:52 I've changed this to profile0 now, see the explana

On 2016/03/09 23:42:10, servolk wrote: > On 2016/03/09 23:30:09, ddorwin wrote: > > On 2016/03/09 22:27:42, servolk wrote: > > > On 2016/03/09 22:20:05, ddorwin wrote: > > > > On 2016/03/09 21:52:40, servolk wrote: > > > > > On 2016/03/09 21:48:47, DaleCurtis wrote: > > > > > > On 2016/03/09 at 19:32:33, servolk wrote: > > > > > > > On 2016/03/09 18:49:47, DaleCurtis wrote: > > > > > > > > Why? > > > > > > > > > > > > > > Well, previously we used VP9_ANY here, which meant potentially you > > could > > > > get > > > > > > any profile, even the highest profile 3. So to minimize a chance of > > > > > regressions > > > > > > I've chosen to use profile3 here (if we used profile0 to initialize > the > > > > > > video_decoder_, then potentially it could refuse to accept higher > > profiles > > > > > later > > > > > > on, but I think if we use profile3 to init the decoder, then using > > > > > lower-profile > > > > > > streams later on should be fine). I think I should actually do the > same > > in > > > > > > media/ffmpeg/ffmpeg_common.cc and > > media/formats/webm/webm_video_client.cc, > > > > > i.e. > > > > > > use the highest profile, instead of profile0). > > > > > > > > > > > > I think your reasoning is sound, but was calling out that this is > > > > inconsistent > > > > > > with what you do elsewhere. > > > > > > > > > > Ok, I've made a change to use profile3 in ffmpeg_common and > > > webm_video_client > > > > in > > > > > the latest patchset. > > > > > > > > IIUC, there is not currently a way to specify other profiles in WebM. > Thus, > > we > > > > should assume in those other places that it is profile 0. > > > > > > > > I don't have any thoughts either way on this file. > > > > > > > > Another item to be aware of is that some of these are eventually reported > as > > > > UMAs. If we go with something other than PROFILE0, we'll see step > functions > > in > > > > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but > > it's > > > > something to be aware of. > > > > > > Yes, currently we won't use any other VP9 profiles other than those > hardcoded > > in > > > this CL, but hopefully it's going to change soon, as I'm planning to look > into > > > reading VP9 profile from the bitstream (or maybe we can just wait until VP9 > > > profile is also provided in codec private data? > > > https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6). > > > And yes, good point about UMA. Currently all profiles get reported as a > value > > > that is going to be used for just PROFILE0 after this CL. But I think this > is > > > the best we can do, no way to adjust that data retroactively. > > > > My point is that any data that does not specify the profile in the private > data > > must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until we > > have such support. Also, all existing content is likely profile 0. > > I agree that most existing content is likely profile 0. But I'm not sure if I > agree that we must assume profile0. All VP9 streams do specify the profile in > bitstream header, it's just that currently we don't read it and so we treat any > stream as being profile VP9_ANY. That wasn't ideal, but that allowed any profile > to work, even profiles 1,2,3. If we used profile0 here, then the video decoder > would be initialized with a set of parameters for profile0 and would most likely > fail to decode higher profiles. But if we use profile3 here, then the decoder > will be ready to decode profile3 and (I'm guessing here) should be also able to > decode lower profiles as well, since all lower profiles seem to be subsets of > higher profiles. So it should keep things working for all VP9 streams and it > shouldn't be any worse than the current situation. > The same reasoning applies to the other two places.

I've changed this to profile0 now, see the explanations at https://codereview.chromium.org/1769593002/#msg23

break;

default:

NOTREACHED();

« no previous file with comments | « ppapi/tests/test_video_encoder.cc ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »