Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1852523002:
Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac (Closed)
|
Created:
4 years, 8 months ago by emircan Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac
This CL enables using VideoToolbox encoder when
kWebRtcH264WithOpenH264FFmpeg flag is on.
BUG=597334
Committed: https://crrev.com/e123143c71aae717dca25f3e2767994b3098ccc9
Cr-Commit-Position: refs/heads/master@{#385860}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Rebase. #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== new flags tied. BUG= ========== to ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac BUG=597334 ==========
emircan@chromium.org changed reviewers: + hbos@chromium.org, niklase@chromium.org
Description was changed from ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac BUG=597334 ========== to ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac This CL enables using VideoToolbox encoder when kWebRtcH264WithOpenH264FFmpeg flag is on. BUG=597334 ==========
PTAL.
We should enable #disable-webrtc-hw-encoding for OSX, that way we can still test both implementations. On 2016/03/31 19:38:20, emircan wrote: > PTAL.
Is this right to do? I'm thinking... 1) kWebRtcH264WithOpenH264FFmpeg is named as such because it it adds implementations using OpenH264 and FFmpeg, that it would also enable a separate HW decoder implementation is counter-intuitive, though it's just the name of the flag. 2) With this, I believe you could explicitly disable HW decoding with the flag and still get it if you've enabled my SW implementation. Why do we want to reuse this flag as such? If we change to only have one flag, shouldn't the other one be removed? Why not keep two flags?
I think we only should have one user facing (chrome://flags) flag to enable/disable H264 for WebRTC. On 2016/04/01 10:12:49, hbos_chromium wrote: > Is this right to do? I'm thinking... > 1) kWebRtcH264WithOpenH264FFmpeg is named as such because it it adds > implementations using OpenH264 and FFmpeg, that it would also enable a separate > HW decoder implementation is counter-intuitive, though it's just the name of the > flag. > 2) With this, I believe you could explicitly disable HW decoding with the flag > and still get it if you've enabled my SW implementation. > > Why do we want to reuse this flag as such? If we change to only have one flag, > shouldn't the other one be removed? Why not keep two flags?
On 2016/04/01 10:12:49, hbos_chromium wrote: > Why do we want to reuse this flag as such? If we change to only have one flag, > shouldn't the other one be removed? Why not keep two flags? As Niklas said, this can be the only user facing flag to enable/disable H264 encoding. If you are talking about "kEnableWebRtcHWH264Encoding" to be removed, I agree that it should be removed eventually. But we need to extensively test it. Currently, it helps to disable V4L2 and VAAPI H264 encode(ChromeOS).
To have a single user facing H.264 about_flag would be nice, I agree. But then I think we should rename it if we have a single flag for H.264. Can this wait though? There are still several H.264 bugs relating to my implementation and an experiment in Canary using my flag (keeping it in dev/beta channels, not in release). For those users who have "my" H.264 enabled, they may already get the HW decoder instead of my SW one. With this change they may also get the HW encoder. This is what we want in the end, HW > SW, but on Macs with HW support my experiment would neither test my encoder or my decoder so I'd get misleading feedback/stats/bug reports on Mac. Currently there's no good way of telling if HW or SW was used for a given bug without digging deeper, and my implementation is not ready to launch just yet. It's just hairy is all, do you want to merge it now anyway?
On 2016/04/04 08:22:41, hbos_chromium wrote: > To have a single user facing H.264 about_flag would be nice, I agree. But then I > think we should rename it if we have a single flag for H.264. > > Can this wait though? There are still several H.264 bugs relating to my > implementation and an experiment in Canary using my flag (keeping it in dev/beta > channels, not in release). For those users who have "my" H.264 enabled, they may > already get the HW decoder instead of my SW one. With this change they may also > get the HW encoder. This is what we want in the end, HW > SW, but on Macs with > HW support my experiment would neither test my encoder or my decoder so I'd get > misleading feedback/stats/bug reports on Mac. > Currently there's no good way of telling if HW or SW was used for a given bug > without digging deeper, and my implementation is not ready to launch just yet. > > It's just hairy is all, do you want to merge it now anyway? I understand that you want to have more testing and exposure of the SW code, and it makes sense. We want to have the same exposure and testing for HW encode in M51 as well. Unfortunately, these two features are not mutually exclusive. Even if we were to make a separate flag and Finch experiment and go through separate feature reviews, HW would override SW. In the long term HW encoder is going to override SW either way. So, I think it would make more sense to tie it up now. IF you think we should change the flag name, I can do it in this CL as well.
On 2016/04/05 23:49:30, emircan wrote: > On 2016/04/04 08:22:41, hbos_chromium wrote: > > To have a single user facing H.264 about_flag would be nice, I agree. But then > I > > think we should rename it if we have a single flag for H.264. > > > > Can this wait though? There are still several H.264 bugs relating to my > > implementation and an experiment in Canary using my flag (keeping it in > dev/beta > > channels, not in release). For those users who have "my" H.264 enabled, they > may > > already get the HW decoder instead of my SW one. With this change they may > also > > get the HW encoder. This is what we want in the end, HW > SW, but on Macs with > > HW support my experiment would neither test my encoder or my decoder so I'd > get > > misleading feedback/stats/bug reports on Mac. > > Currently there's no good way of telling if HW or SW was used for a given bug > > without digging deeper, and my implementation is not ready to launch just yet. > > > > It's just hairy is all, do you want to merge it now anyway? > > I understand that you want to have more testing and exposure of the SW code, and > it makes sense. We want to have the same exposure and testing for HW encode in > M51 as well. Unfortunately, these two features are not mutually exclusive. Even > if we were to make a separate flag and Finch experiment and go through separate > feature reviews, HW would override SW. In the long term HW encoder is going to > override SW either way. So, I think it would make more sense to tie it up now. > IF you think we should change the flag name, I can do it in this CL as well. Hmm, okay then. You're right they're not mutually exclusive and having both is more like testing the real thing. The experiment would then test more implementations, and HW encode bugs may be discovered using the flag, but SW encode will have a good coverage either way. However, with a change like this, HW encode bugs would block SW encode/decode from launching... (We couldn't switch the default value of the flag to true if there are significant HW encode bugs.) There is some pressure to get H.264 launched ASAP. We can use my flag to enable both implementations. Hopefully this doesn't happen, but what if SW is ready to launch before HW? Would we then revert this and use a separate flag? If we proceed I'm fine with not renaming the flag for now since the flag has had some visibility (been mentioned in forums and bugs and has been used externally) and the experiment uses the current name (I'm guessing it would have to be updated to use both old and new flag names for the transition since the configuration file would be used by multiple versions).
lgtm if in the event of outstanding HW encode bugs we make sure HW encode bugs does not block SW encode/decode from launching. https://codereview.chromium.org/1852523002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1852523002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5555: + When enabled, an H.264 software video encoder/decoder pair is included. (If a hardware encoder/decoder is also available it may be used instead of this decoder). I think the parenthesis can be removed. Also "instead of this decoder" should be "instead of this encoder/decoder". Or "If a hardware encoder or decoder is also available, they may be used instead of the software ones."
Yes, if we see HW errors blocking the process, we can definitely revert this and continue in a separate HW experiment. Thanks. https://codereview.chromium.org/1852523002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1852523002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5555: + When enabled, an H.264 software video encoder/decoder pair is included. (If a hardware encoder/decoder is also available it may be used instead of this decoder). On 2016/04/06 10:05:39, hbos_chromium wrote: > I think the parenthesis can be removed. Also "instead of this decoder" should be > "instead of this encoder/decoder". > > Or "If a hardware encoder or decoder is also available, they may be used instead > of the software ones." Done.
emircan@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@ for OWNERS review.
LGTM, please update the bug link. https://codereview.chromium.org/1852523002/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/1852523002/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder_factory.cc:40: base::FeatureList::IsEnabled(content::kWebRtcH264WithOpenH264FFmpeg); No need for content::
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1852523002/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/1852523002/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder_factory.cc:40: base::FeatureList::IsEnabled(content::kWebRtcH264WithOpenH264FFmpeg); On 2016/04/07 17:14:42, mcasas wrote: > No need for content:: Done.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1852523002/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1852523002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1852523002/60001
Message was sent while issue was closed.
Description was changed from ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac This CL enables using VideoToolbox encoder when kWebRtcH264WithOpenH264FFmpeg flag is on. BUG=597334 ========== to ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac This CL enables using VideoToolbox encoder when kWebRtcH264WithOpenH264FFmpeg flag is on. BUG=597334 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac This CL enables using VideoToolbox encoder when kWebRtcH264WithOpenH264FFmpeg flag is on. BUG=597334 ========== to ========== Tie kWebRtcH264WithOpenH264FFmpeg and kEnableWebRtcHWH264Encoding flags on Mac This CL enables using VideoToolbox encoder when kWebRtcH264WithOpenH264FFmpeg flag is on. BUG=597334 Committed: https://crrev.com/e123143c71aae717dca25f3e2767994b3098ccc9 Cr-Commit-Position: refs/heads/master@{#385860} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e123143c71aae717dca25f3e2767994b3098ccc9 Cr-Commit-Position: refs/heads/master@{#385860}
Message was sent while issue was closed.
On 2016/04/07 21:29:50, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/e123143c71aae717dca25f3e2767994b3098ccc9 > Cr-Commit-Position: refs/heads/master@{#385860} FTR: This was reverted in https://codereview.chromium.org/1868133002 See https://bugs.chromium.org/p/chromium/issues/detail?id=600685 for details.
Message was sent while issue was closed.
Patchset #4 (id:80001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
