Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573933002:
Restore switch kDisableWebRtcHWEncoding (Closed)
|
Created:
4 years ago by braveyao Modified:
3 years, 11 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, alemate+watch_chromium.org, creis+watch_chromium.org, oshima+watch_chromium.org, posciak+watch_chromium.org, jam, nasko+codewatch_chromium.org, achuith+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org, mcasas+watch+vc_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestore switch kDisableWebRtcHWEncoding
In cl https://codereview.chromium.org/2549283002/, kDisableWebRtcHWEncoding was
renamed to kDisableWebRtcHWVP8Encoding by the review comments. And as posciak@
mentioned later in that cl:
-------------------------------------------------------------------------------
kDisableWebRtcHWEncoding was essential for Chrome OS as we had an ability to
have a global switch to disable all HW encoding with it, including both H264 and
VP8 encoding.
It's very useful and widely documented and known, and used by QA and developers.
We would really prefer to keep it please.
-------------------------------------------------------------------------------
So kDisableWabRtcHWEncoding is restored here.
BUG=664652
Review-Url: https://codereview.chromium.org/2573933002
Cr-Commit-Position: refs/heads/master@{#444102}
Committed: https://chromium.googlesource.com/chromium/src/+/e29be16f1013dfa1f3f5f2e8108152aca312fa9a
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comment in histograms.xml #Patch Set 3 : check kDisableWebRtcHWEncoding only at creating encoder_factory #
Total comments: 24
Patch Set 4 : keep switches in right order #
Total comments: 2
Patch Set 5 : rebase to Dec21 #Patch Set 6 : make H264 flag available on CrOS. #
Total comments: 2
Patch Set 7 : renew comments #Messages
Total messages: 64 (41 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + jam@chromium.org, posciak@chromium.org
Hi jam@, please take a look when you get a chance.
lgtm
braveyao@chromium.org changed reviewers: + jwd@chromium.org
Hi jwd@, need owner's review to histograms.xml again. sorry for the back and forth.
https://codereview.chromium.org/2573933002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2573933002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92783: - <int value="-1948540128" label="disable-webrtc-hw-encoding (deprecated)"/> Can you keep this here, otherwise old data might not get labeled correctly.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi jwd@, PTAL. https://codereview.chromium.org/2573933002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2573933002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:92783: - <int value="-1948540128" label="disable-webrtc-hw-encoding (deprecated)"/> On 2016/12/14 18:10:03, jwd wrote: > Can you keep this here, otherwise old data might not get labeled correctly. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for the patch. Would it however be possible to keep checking only for kDisableWebRtcHWEncoding everywhere as before, and only use the codec-specific flags in the factories, to simplify the logic?
On 2016/12/15 00:15:41, Pawel Osciak wrote: > Thank you for the patch. Would it however be possible to keep checking only for > kDisableWebRtcHWEncoding everywhere as before, and only use the codec-specific > flags in the factories, to simplify the logic? Could you please be more specific? Do you mean we always create VEA on Android even if no VP8 or H264 is enabled?
On 2016/12/15 00:34:45, braveyao wrote: > On 2016/12/15 00:15:41, Pawel Osciak wrote: > > Thank you for the patch. Would it however be possible to keep checking only > for > > kDisableWebRtcHWEncoding everywhere as before, and only use the codec-specific > > flags in the factories, to simplify the logic? > > Could you please be more specific? Do you mean we always create VEA on Android > even if no VP8 or H264 is enabled? Before https://codereview.chromium.org/2549283002, we used to check only for kDisableWebRtcHWEncoding to make a decision whether to create an RTCVideoEncoderFactory. Could we perhaps keep doing that, i.e. keep checking for only kDisableWebRtcHWEncoding there and in other places where we check for both kDisableWebRtcHWEncoding and codec-specific flags, and only check codec-specific flags in the factory itself?
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
posciak@, thanks for the advice! Creating an encoder factory with no codec enabled also works fine. PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, This flag is marked here, as well as described in content_features.cc, as only for Android (although it is actually enabled and being used outside #if defined(OS_ANDROID) in this CL as far as I can see). We need to use it outside of Android to keep H264 enabled for CrOS as well. On the other hand, I think your original intention was to disable only VP8, so perhaps this flag is not needed at all and you should just be passing kDisableWebRtcHWVP8Encoding for your use case? https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); Could we use kDisableWebRtcHWEncoding only here, and use kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:122: switches::kDisableWebRtcHWEncoding, Do we need to propagate all the flags? https://codereview.chromium.org/2573933002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1797: switches::kDisableWebRtcHWEncoding, And here as well? https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... content/public/browser/gpu_utils.cc:50: command_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || Could we keep only kDisableWebRtcHWEncoding here as well please? https://codereview.chromium.org/2573933002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/common/c... content/public/common/content_switches.h:268: CONTENT_EXPORT extern const char kDisableWebRtcHWDecoding[]; Nit: could we keep lexicographical ordering please?
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
posciak@, please check my replies and offer further opinion. The major concern is if we want to create VEA on Android if H264 is diabled. Any negative impact if we create something even knowing we are not going to use it? https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, On 2016/12/15 05:45:53, Pawel Osciak wrote: > This flag is marked here, as well as described in content_features.cc, as only > for Android (although it is actually enabled and being used outside #if > defined(OS_ANDROID) in this CL as far as I can see). > > We need to use it outside of Android to keep H264 enabled for CrOS as well. > > On the other hand, I think your original intention was to disable only VP8, so > perhaps this flag is not needed at all and you should just be passing > kDisableWebRtcHWVP8Encoding for your use case? #enable-webrtc-hw-h264-encoding is ENABLED by default on CrOS and Android(and other platforms). So it won't affect CrOS H264. Here we only add it in about_flag page on Android to offer a way to disable it manually. We need this feature flag. Because we need to control the percentage of deployment if anything goes very wrong. https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); On 2016/12/15 05:45:53, Pawel Osciak wrote: > Could we use kDisableWebRtcHWEncoding only here, and use > kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? It depends on how we decide to create VEA on Android. At present since kDisalbeWebRtcHWEncoding will disable both VP8 and H264, then no VEA will be created if this switch is appended. See my other comments in gpu_utils.cc. https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:122: switches::kDisableWebRtcHWEncoding, On 2016/12/15 05:45:53, Pawel Osciak wrote: > Do we need to propagate all the flags? Sorry I can't answer this question. I just restore it to be same as before. If anyone could confirm it's not needed here, I can help to remove it. BTW: test shows that removing them is OK. https://codereview.chromium.org/2573933002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:1797: switches::kDisableWebRtcHWEncoding, On 2016/12/15 05:45:53, Pawel Osciak wrote: > And here as well? Same answer here as I just restore my change to be same as before. If anyone could confirm they are not needed, I can help to remove these two, HWDecoding/Encoding. https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... content/public/browser/gpu_utils.cc:50: command_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || On 2016/12/15 05:45:53, Pawel Osciak wrote: > Could we keep only kDisableWebRtcHWEncoding here as well please? Probably we can. The consequence, as I mentioned yesterday, will be if we disable H264(not append kDisableWebRtcHWEncoding. And so far VP8 is disabled by default), then VEA will be created even no one will use it. This changed the previous behavior. What if we always create VEA on Android? Will it add CPU load or power consumption? I suppose the principle is we only create the resource when we about to use it. https://codereview.chromium.org/2573933002/diff/40001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/common/c... content/public/common/content_switches.h:268: CONTENT_EXPORT extern const char kDisableWebRtcHWDecoding[]; On 2016/12/15 05:45:53, Pawel Osciak wrote: > Nit: could we keep lexicographical ordering please? Done. It's in bad order all the time. Sorry for not noticing it, even modifying codes here several times...
histograms lgtm
https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, On 2016/12/15 20:46:24, braveyao wrote: > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > This flag is marked here, as well as described in content_features.cc, as only > > for Android (although it is actually enabled and being used outside #if > > defined(OS_ANDROID) in this CL as far as I can see). > > > > We need to use it outside of Android to keep H264 enabled for CrOS as well. > > > > On the other hand, I think your original intention was to disable only VP8, so > > perhaps this flag is not needed at all and you should just be passing > > kDisableWebRtcHWVP8Encoding for your use case? > > #enable-webrtc-hw-h264-encoding is ENABLED by default on CrOS and Android(and > other platforms). So it won't affect CrOS H264. Here we only add it in > about_flag page on Android to offer a way to disable it manually. > > We need this feature flag. Because we need to control the percentage of > deployment if anything goes very wrong. Yes, I agree with that. What I meant was that this change changes "kOsAndroid | kOsCrOS" to "kOsAndroid" only, even though it's not Android-only. As for the deployment switch, if you needed to disable H264 encode, would you still want to use VP8 HW encode? If not, you could use kDisableWebRtcHWEncoding as a disable flag instead of this... https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); On 2016/12/15 20:46:24, braveyao wrote: > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > Could we use kDisableWebRtcHWEncoding only here, and use > > kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? > > It depends on how we decide to create VEA on Android. At present since > kDisalbeWebRtcHWEncoding will disable both VP8 and H264, then no VEA will be > created if this switch is appended. Yes, that's what should be expected from the name of the flag, kDisableWebRtcHWEncoding should be disabling all HW WebRTC encode features. Please also keep in mind this is not Android-only, so the decision here is not only affecting VEA creation on Android, but on other systems as well. https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:122: switches::kDisableWebRtcHWEncoding, On 2016/12/15 20:46:24, braveyao wrote: > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > Do we need to propagate all the flags? > > Sorry I can't answer this question. I just restore it to be same as before. > If anyone could confirm it's not needed here, I can help to remove it. > BTW: test shows that removing them is OK. How did you test? Were the codec-specific flags propagated to GPU and renderer processes? Were you able to selectively disable each codec with each flag alone (i.e. only VP8 with kDisableWebRtcHWVP8Encoding and only H264 with kWebRtcHWH264Encoding), while having the other HW codec working still? https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... content/public/browser/gpu_utils.cc:50: command_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || On 2016/12/15 20:46:24, braveyao wrote: > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > Could we keep only kDisableWebRtcHWEncoding here as well please? > > Probably we can. The consequence, as I mentioned yesterday, will be if we > disable H264(not append kDisableWebRtcHWEncoding. And so far VP8 is disabled by > default), then VEA will be created even no one will use it. This changed the > previous behavior. These switches control not only Android. VP8 encode is enabled by default on CrOS. > What if we always create VEA on Android? Will it add CPU load or power > consumption? I suppose the principle is we only create the resource when we > about to use it. Sorry, I'm not familiar how this would affect Android in particular for power/CPU, but if you needed to disable encoding in general and not create a VEA at all, kDisableWebRtcHWEncoding should be more appropriate, instead of a codec-specific flag.
Hi posciak@, comments all answered. Again, the changes in this cl are for this purpose: Restore kDisableWebRtcHWEncoding, then behavior is totally same as before on CrOS. On Android, VP8 and H264 can be controlled selectively with each flag. kDisableWebRtcHWEncoding can still disable all HW encoding as before, regardless the two codec-specific flag. As to VEA creation, no VEA will be created if kDisableWebRtcHWEncoding is appended, or none of VP8/H264 is enabled. https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, On 2016/12/20 01:49:17, Pawel Osciak wrote: > On 2016/12/15 20:46:24, braveyao wrote: > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > This flag is marked here, as well as described in content_features.cc, as > only > > > for Android (although it is actually enabled and being used outside #if > > > defined(OS_ANDROID) in this CL as far as I can see). > > > > > > We need to use it outside of Android to keep H264 enabled for CrOS as well. > > > > > > On the other hand, I think your original intention was to disable only VP8, > so > > > perhaps this flag is not needed at all and you should just be passing > > > kDisableWebRtcHWVP8Encoding for your use case? > > > > #enable-webrtc-hw-h264-encoding is ENABLED by default on CrOS and Android(and > > other platforms). So it won't affect CrOS H264. Here we only add it in > > about_flag page on Android to offer a way to disable it manually. > > > > We need this feature flag. Because we need to control the percentage of > > deployment if anything goes very wrong. > > Yes, I agree with that. What I meant was that this change changes "kOsAndroid | > kOsCrOS" to "kOsAndroid" only, even though it's not Android-only. > > As for the deployment switch, if you needed to disable H264 encode, would you > still want to use VP8 HW encode? If not, you could use kDisableWebRtcHWEncoding > as a disable flag instead of this... Yes I changed it back to kOsAndroid only since kDisableWebRtcHWEncoding is restored here. Now on CrOS, nothing is changed. On Android, VP8 and H264 can be enable/disable respectively (which is by design), also can be both disabled by kDisableWebRtcHWEncoding as before. https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); On 2016/12/20 01:49:17, Pawel Osciak wrote: > On 2016/12/15 20:46:24, braveyao wrote: > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > Could we use kDisableWebRtcHWEncoding only here, and use > > > kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? > > > > It depends on how we decide to create VEA on Android. At present since > > kDisalbeWebRtcHWEncoding will disable both VP8 and H264, then no VEA will be > > created if this switch is appended. > > Yes, that's what should be expected from the name of the flag, > kDisableWebRtcHWEncoding should be disabling all HW WebRTC encode features. > Please also keep in mind this is not Android-only, so the decision here is not > only affecting VEA creation on Android, but on other systems as well. "IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE" is only valid on Android. https://cs.chromium.org/chromium/src/gpu/config/software_rendering_list_json.... https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_process_host.cc:122: switches::kDisableWebRtcHWEncoding, On 2016/12/20 01:49:17, Pawel Osciak wrote: > On 2016/12/15 20:46:24, braveyao wrote: > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > Do we need to propagate all the flags? > > > > Sorry I can't answer this question. I just restore it to be same as before. > > If anyone could confirm it's not needed here, I can help to remove it. > > BTW: test shows that removing them is OK. > > How did you test? Were the codec-specific flags propagated to GPU and renderer > processes? Were you able to selectively disable each codec with each flag alone > (i.e. only VP8 with kDisableWebRtcHWVP8Encoding and only H264 with > kWebRtcHWH264Encoding), while having the other HW codec working still? Yes VP8 and H264 can be selectively disabled with each flag. And I don't think the codec-specific flags need to be propagated between GPU and renderer processes. So I suppose no change here. https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/public/browser/... content/public/browser/gpu_utils.cc:50: command_line->HasSwitch(switches::kDisableWebRtcHWEncoding) || On 2016/12/20 01:49:17, Pawel Osciak wrote: > On 2016/12/15 20:46:24, braveyao wrote: > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > Could we keep only kDisableWebRtcHWEncoding here as well please? > > > > Probably we can. The consequence, as I mentioned yesterday, will be if we > > disable H264(not append kDisableWebRtcHWEncoding. And so far VP8 is disabled > by > > default), then VEA will be created even no one will use it. This changed the > > previous behavior. > > These switches control not only Android. VP8 encode is enabled by default on > CrOS. > > > What if we always create VEA on Android? Will it add CPU load or power > > consumption? I suppose the principle is we only create the resource when we > > about to use it. > > Sorry, I'm not familiar how this would affect Android in particular for > power/CPU, but if you needed to disable encoding in general and not create a VEA > at all, kDisableWebRtcHWEncoding should be more appropriate, instead of a > codec-specific flag. Again on CrOS, nothing is changed. You control with kDisableWebRtcHWEncoing only. On Android, we can control VEA creation not only by kDisableWebRtcHWEncoding as before, but also by checking if none of VP8/H264 is enabled. This just covers another use case.
On 2016/12/20 19:09:14, braveyao wrote: > Hi posciak@, comments all answered. > Again, the changes in this cl are for this purpose: > > Restore kDisableWebRtcHWEncoding, then behavior is totally same as before on > CrOS. I'm sorry for being so detailed, but I think we used to be able to disable codecs selectively using kDisableWebRtcHWEncoding=codecname, however now we are making the H264 flag Android-only? This is useful to have on CrOS, for the same reasons as on Android. > On Android, VP8 and H264 can be controlled selectively with each flag. It would be useful if we did not limit this per-codec control to Android only, but allow this control on other systems as well. > kDisableWebRtcHWEncoding can still disable all HW encoding as before, regardless > the two codec-specific flag. > As to VEA creation, no VEA will be created if kDisableWebRtcHWEncoding is > appended, or none of VP8/H264 is enabled. This is what I'm concerned about, disabling H264 and VP8 would disable all HW encoding, but there could be other codecs as well. That's also different from the state before the previous CL I believe, even if right now it may be functionally equivalent for systems without support for other codecs, it may change in the future. https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, On 2016/12/20 19:09:14, braveyao wrote: > On 2016/12/20 01:49:17, Pawel Osciak wrote: > > On 2016/12/15 20:46:24, braveyao wrote: > > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > > This flag is marked here, as well as described in content_features.cc, as > > only > > > > for Android (although it is actually enabled and being used outside #if > > > > defined(OS_ANDROID) in this CL as far as I can see). > > > > > > > > We need to use it outside of Android to keep H264 enabled for CrOS as > well. > > > > > > > > On the other hand, I think your original intention was to disable only > VP8, > > so > > > > perhaps this flag is not needed at all and you should just be passing > > > > kDisableWebRtcHWVP8Encoding for your use case? > > > > > > #enable-webrtc-hw-h264-encoding is ENABLED by default on CrOS and > Android(and > > > other platforms). So it won't affect CrOS H264. Here we only add it in > > > about_flag page on Android to offer a way to disable it manually. > > > > > > We need this feature flag. Because we need to control the percentage of > > > deployment if anything goes very wrong. > > > > Yes, I agree with that. What I meant was that this change changes "kOsAndroid > | > > kOsCrOS" to "kOsAndroid" only, even though it's not Android-only. > > > > As for the deployment switch, if you needed to disable H264 encode, would you > > still want to use VP8 HW encode? If not, you could use > kDisableWebRtcHWEncoding > > as a disable flag instead of this... > > Yes I changed it back to kOsAndroid only since kDisableWebRtcHWEncoding is > restored here. > Now on CrOS, nothing is changed. > On Android, VP8 and H264 can be enable/disable respectively (which is by > design), also can be both disabled by kDisableWebRtcHWEncoding as before. I believe before your previous change we could pass a codec name to kDisableWebRtcHWEncoding to selectively disable codecs... I'm ok using explicit flags for this instead of a value, but it would be great if that worked not only on Android... https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); On 2016/12/20 19:09:14, braveyao wrote: > On 2016/12/20 01:49:17, Pawel Osciak wrote: > > On 2016/12/15 20:46:24, braveyao wrote: > > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > > Could we use kDisableWebRtcHWEncoding only here, and use > > > > kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? > > > > > > It depends on how we decide to create VEA on Android. At present since > > > kDisalbeWebRtcHWEncoding will disable both VP8 and H264, then no VEA will be > > > created if this switch is appended. > > > > Yes, that's what should be expected from the name of the flag, > > kDisableWebRtcHWEncoding should be disabling all HW WebRTC encode features. > > Please also keep in mind this is not Android-only, so the decision here is not > > only affecting VEA creation on Android, but on other systems as well. > > "IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE" is only > valid on Android. > https://cs.chromium.org/chromium/src/gpu/config/software_rendering_list_json.... Regardless of what system this is for, if there are other codecs than VP8 and H264, then I'm worried it will be surprising that this doesn't work as expected (i.e. disabling VP8 and H264 disables all HW encoding). By the way, I'm not sure about this, but if this is only for Android, should it be under #if defined(OS_ANDROID)? https://codereview.chromium.org/2573933002/diff/60001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/60001/content/public/browser/... content/public/browser/gpu_utils.cc:52: !base::FeatureList::IsEnabled(features::kWebRtcHWH264Encoding)); Previously we disabled this only based on kDisableWebRtcHWEncoding, now we disable also if it's not set, but H264 and VP8 are disabled. But we may also have other codecs in use, such as VP9.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Hi posciak@, enabled H264 flag for CrOS and adjusted the logic in gpu_data_manager_impl_private.cc to be exact same as before. PTAL. https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2573933002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:710: IDS_FLAGS_WEBRTC_HW_H264_ENCODING_DESCRIPTION, kOsAndroid, On 2016/12/21 01:12:57, Pawel Osciak wrote: > On 2016/12/20 19:09:14, braveyao wrote: > > On 2016/12/20 01:49:17, Pawel Osciak wrote: > > > On 2016/12/15 20:46:24, braveyao wrote: > > > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > > > This flag is marked here, as well as described in content_features.cc, > as > > > only > > > > > for Android (although it is actually enabled and being used outside #if > > > > > defined(OS_ANDROID) in this CL as far as I can see). > > > > > > > > > > We need to use it outside of Android to keep H264 enabled for CrOS as > > well. > > > > > > > > > > On the other hand, I think your original intention was to disable only > > VP8, > > > so > > > > > perhaps this flag is not needed at all and you should just be passing > > > > > kDisableWebRtcHWVP8Encoding for your use case? > > > > > > > > #enable-webrtc-hw-h264-encoding is ENABLED by default on CrOS and > > Android(and > > > > other platforms). So it won't affect CrOS H264. Here we only add it in > > > > about_flag page on Android to offer a way to disable it manually. > > > > > > > > We need this feature flag. Because we need to control the percentage of > > > > deployment if anything goes very wrong. > > > > > > Yes, I agree with that. What I meant was that this change changes > "kOsAndroid > > | > > > kOsCrOS" to "kOsAndroid" only, even though it's not Android-only. > > > > > > As for the deployment switch, if you needed to disable H264 encode, would > you > > > still want to use VP8 HW encode? If not, you could use > > kDisableWebRtcHWEncoding > > > as a disable flag instead of this... > > > > Yes I changed it back to kOsAndroid only since kDisableWebRtcHWEncoding is > > restored here. > > Now on CrOS, nothing is changed. > > On Android, VP8 and H264 can be enable/disable respectively (which is by > > design), also can be both disabled by kDisableWebRtcHWEncoding as before. > > I believe before your previous change we could pass a codec name to > kDisableWebRtcHWEncoding to selectively disable codecs... I'm ok using explicit > flags for this instead of a value, but it would be great if that worked not only > on Android... Done. I can make the H264 flag also available on CrOS. But I wondering how you can "disable codecs selectively using kDisableWebRtcHWEncoding=codecname" on CrOS before. I didn't see such logic in the codes. https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:780: command_line->AppendSwitch(switches::kDisableWebRtcHWVP8Encoding); On 2016/12/21 01:12:57, Pawel Osciak wrote: > On 2016/12/20 19:09:14, braveyao wrote: > > On 2016/12/20 01:49:17, Pawel Osciak wrote: > > > On 2016/12/15 20:46:24, braveyao wrote: > > > > On 2016/12/15 05:45:53, Pawel Osciak wrote: > > > > > Could we use kDisableWebRtcHWEncoding only here, and use > > > > > kDisableWebRtcHWVP8Encoding in the factory in codec choice instead? > > > > > > > > It depends on how we decide to create VEA on Android. At present since > > > > kDisalbeWebRtcHWEncoding will disable both VP8 and H264, then no VEA will > be > > > > created if this switch is appended. > > > > > > Yes, that's what should be expected from the name of the flag, > > > kDisableWebRtcHWEncoding should be disabling all HW WebRTC encode features. > > > Please also keep in mind this is not Android-only, so the decision here is > not > > > only affecting VEA creation on Android, but on other systems as well. > > > > "IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE" is only > > valid on Android. > > > https://cs.chromium.org/chromium/src/gpu/config/software_rendering_list_json.... > > Regardless of what system this is for, if there are other codecs than VP8 and > H264, then I'm worried it will be surprising that this doesn't work as expected > (i.e. disabling VP8 and H264 disables all HW encoding). > > By the way, I'm not sure about this, but if this is only for Android, should it > be under #if defined(OS_ANDROID)? Done. GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE is only blacklisted on Android, so no need the "#if defined(OS_ANDROID)". And yes, on Android, only VP8 and H264 are available so far. To make this part as same as before, I move all processing under the IsFeatureBlacklisted() section again. PTAL. https://codereview.chromium.org/2573933002/diff/60001/content/public/browser/... File content/public/browser/gpu_utils.cc (right): https://codereview.chromium.org/2573933002/diff/60001/content/public/browser/... content/public/browser/gpu_utils.cc:52: !base::FeatureList::IsEnabled(features::kWebRtcHWH264Encoding)); On 2016/12/21 01:12:57, Pawel Osciak wrote: > Previously we disabled this only based on kDisableWebRtcHWEncoding, now we > disable also if it's not set, but H264 and VP8 are disabled. But we may also > have other codecs in use, such as VP9. No. For RTC, only VP8 and H264 are available on all platforms. See https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_enc... So keep this to save some resources on Android.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for forgetting upload the rebase first. Separated yesterdays patchset into two for easy reviewing. Again, on Android I prefer the two codec specific flags over kDisableWebRtcHWEncoding(which is basically only for consistency on CrOS.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2573933002/diff/120001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:780: if (IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE) && Sorry, I'm confused by the intended logic of this block. The original logic was to append kDisableWebRtcHWEncoding if encoding was blacklisted, but the kDisableWebRtcHWEncoding switch was not on commandline (https://chromium.googlesource.com/chromium/src/+blame/71d41b68d19128f43405820...). Now, unless I'm misreading something, it seems that we'll only append kDisableWebRtcHWEncoding if kDisableWebRtcHWVP8Encoding is not on commandline. However, if kDisableWebRtcHWVP8Encoding is on commandline, then we won't append kDisableWebRtcHWEncoding regardless of anything else? Could you explain the intention of the whole block and add comments please?
https://codereview.chromium.org/2573933002/diff/120001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2573933002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:780: if (IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_ACCELERATED_VIDEO_ENCODE) && On 2017/01/11 09:55:35, Pawel Osciak wrote: > Sorry, I'm confused by the intended logic of this block. > > The original logic was to append kDisableWebRtcHWEncoding if encoding was > blacklisted, but the kDisableWebRtcHWEncoding switch was not on commandline > (https://chromium.googlesource.com/chromium/src/+blame/71d41b68d19128f43405820...). > > Now, unless I'm misreading something, it seems that we'll only append > kDisableWebRtcHWEncoding if kDisableWebRtcHWVP8Encoding is not on commandline. > However, if kDisableWebRtcHWVP8Encoding is on commandline, then we won't append > kDisableWebRtcHWEncoding regardless of anything else? > > Could you explain the intention of the whole block and add comments please? First of all, now the blacklist is temporarily for blacklisting HW VP8 only on Android (in the past it will blacklist all HW codec on Android). And on Android, generally you can't append commandline flag by clicking App icon to launch it. So the logic here is mainly to append the switch to disable HW VP8 on Android. And then set gpu_preferences or append kDisableWebRtcHWEncoding depending on HW H264 status. And I'm preparing to remove the blacklist soon to enable HW VP8 for mediaRecording only on Android (no WebRTC yet).
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi posciak, the comments is renewed as the offline discuss. PTAL.
Thank you. lgtm
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2573933002/#ps140001 (title: "renew comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484678860823760,
"parent_rev": "734c246453da8b9a38d7266fe2bbd272037c99e8", "commit_rev":
"e29be16f1013dfa1f3f5f2e8108152aca312fa9a"}
Message was sent while issue was closed.
Description was changed from ========== Restore switch kDisableWebRtcHWEncoding In cl https://codereview.chromium.org/2549283002/, kDisableWebRtcHWEncoding was renamed to kDisableWebRtcHWVP8Encoding by the review comments. And as posciak@ mentioned later in that cl: ------------------------------------------------------------------------------- kDisableWebRtcHWEncoding was essential for Chrome OS as we had an ability to have a global switch to disable all HW encoding with it, including both H264 and VP8 encoding. It's very useful and widely documented and known, and used by QA and developers. We would really prefer to keep it please. ------------------------------------------------------------------------------- So kDisableWabRtcHWEncoding is restored here. BUG=664652 ========== to ========== Restore switch kDisableWebRtcHWEncoding In cl https://codereview.chromium.org/2549283002/, kDisableWebRtcHWEncoding was renamed to kDisableWebRtcHWVP8Encoding by the review comments. And as posciak@ mentioned later in that cl: ------------------------------------------------------------------------------- kDisableWebRtcHWEncoding was essential for Chrome OS as we had an ability to have a global switch to disable all HW encoding with it, including both H264 and VP8 encoding. It's very useful and widely documented and known, and used by QA and developers. We would really prefer to keep it please. ------------------------------------------------------------------------------- So kDisableWabRtcHWEncoding is restored here. BUG=664652 Review-Url: https://codereview.chromium.org/2573933002 Cr-Commit-Position: refs/heads/master@{#444102} Committed: https://chromium.googlesource.com/chromium/src/+/e29be16f1013dfa1f3f5f2e81081... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e29be16f1013dfa1f3f5f2e81081... |
