Resolve the mismatch between WebRTC's default H264 profile choice for HW codecs |
||||||
Issue descriptionWhen choosing profile level for HW encoders/decoders, WebRTC currently chooses default values for H264. This is MAIN for decoder, and BASELINE for encoder. - SW encode happens with CONSTRAINED BASELINE profile in OpenH264, but currently we do not distinguish between CONSTRAINED BASELINE and BASELINE. - Quoting posciak@: HW decoders can mostly handle CONSTRAINED BASELINE and MAIN. - It makes sense to choose MAIN as default profile in HW encode in that case. magjed@ can you tell if this aligns with your work on http://crbug/webrtc/6337? [0] https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_encoder.cc?rcl=c7dda59dad207a6642d7a201e7ed088f3e8bad92&l=49 [1] https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_decoder.cc?rcl=c7dda59dad207a6642d7a201e7ed088f3e8bad92&l=117
,
Feb 7 2017
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/829b1d57525c3c6549d18a2c85a96527d59ea5e9 commit 829b1d57525c3c6549d18a2c85a96527d59ea5e9 Author: magjed <magjed@chromium.org> Date: Fri Jul 07 10:17:45 2017 Reland of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:1 of https://codereview.chromium.org/2521923002/ ) Reason for revert: Try again. Original issue's description: > Revert of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #3 id:60001 of https://codereview.chromium.org/2499973002/ ) > > Reason for revert: > Causes these tests to fail on chromium.webrtc bots for Win and Mac: > WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsH264 > WebRtcVideoQualityBrowserTests/WebRtcVideoQualityBrowserTest.MANUAL_TestVideoQualityH264 > https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/30367 > https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/62661 > > Original issue's description: > > RTCVideoEncoder: Report H264 profile information to WebRTC > > > > This CL updates RTCVideoEncoderFactory to report cricket::VideoCodecs > > instead of WebRtcVideoEncoderFactory::VideoCodecs. The H264 profile > > information is added to the cricket::VideoCodec so that WebRTC receives > > this information. Also, the mapping between media::VideoCodecProfiles > > and cricket::VideoCodecs is cached so that we can send the > > media::VideoCodecProfile to RTCVideoEncoder instead of having to deal > > with webrtc::VideoCodecType. > > > > BUG= webrtc:6337 > > > > Committed: https://crrev.com/510eddede44cb4b67c8f17fdd68cefb780a668c5 > > Cr-Commit-Position: refs/heads/master@{#433508} > > TBR=emircan@chromium.org,posciak@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= webrtc:6337 > > Committed: https://crrev.com/c2564bc627cb950b124ac8e41bc5fd3187f7ad9c > Cr-Commit-Position: refs/heads/master@{#433828} TBR=emircan@chromium.org,posciak@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 688541 , 735959 Review-Url: https://codereview.chromium.org/2548443002 Cr-Commit-Position: refs/heads/master@{#484874} [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/BUILD.gn [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/media/gpu/rtc_video_encoder.cc [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/media/gpu/rtc_video_encoder.h [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/media/gpu/rtc_video_encoder_factory.cc [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/media/gpu/rtc_video_encoder_factory.h [modify] https://crrev.com/829b1d57525c3c6549d18a2c85a96527d59ea5e9/content/renderer/media/gpu/rtc_video_encoder_unittest.cc
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df6e5a5c7e7c665603f9619930a1d7106b55160d commit df6e5a5c7e7c665603f9619930a1d7106b55160d Author: niklase <niklase@chromium.org> Date: Fri Jul 07 16:59:38 2017 Revert of TCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:190001 of https://codereview.chromium.org/2548443002/ ) Reason for revert: Reverting this since it's causing multiple perf regression on mac, looks like HW encode/decode might get disabled. Original issue's description: > Reland of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:1 of https://codereview.chromium.org/2521923002/ ) > > Reason for revert: > Try again. > > Original issue's description: > > Revert of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #3 id:60001 of https://codereview.chromium.org/2499973002/ ) > > > > Reason for revert: > > Causes these tests to fail on chromium.webrtc bots for Win and Mac: > > WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsH264 > > WebRtcVideoQualityBrowserTests/WebRtcVideoQualityBrowserTest.MANUAL_TestVideoQualityH264 > > https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/30367 > > https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/62661 > > > > Original issue's description: > > > RTCVideoEncoder: Report H264 profile information to WebRTC > > > > > > This CL updates RTCVideoEncoderFactory to report cricket::VideoCodecs > > > instead of WebRtcVideoEncoderFactory::VideoCodecs. The H264 profile > > > information is added to the cricket::VideoCodec so that WebRTC receives > > > this information. Also, the mapping between media::VideoCodecProfiles > > > and cricket::VideoCodecs is cached so that we can send the > > > media::VideoCodecProfile to RTCVideoEncoder instead of having to deal > > > with webrtc::VideoCodecType. > > > > > > BUG= webrtc:6337 > > > > > > Committed: https://crrev.com/510eddede44cb4b67c8f17fdd68cefb780a668c5 > > > Cr-Commit-Position: refs/heads/master@{#433508} > > > > TBR=emircan@chromium.org,posciak@chromium.org > > # Skipping CQ checks because original CL landed less than 1 days ago. > > NOPRESUBMIT=true > > NOTREECHECKS=true > > NOTRY=true > > BUG= webrtc:6337 > > > > Committed: https://crrev.com/c2564bc627cb950b124ac8e41bc5fd3187f7ad9c > > Cr-Commit-Position: refs/heads/master@{#433828} > > TBR=emircan@chromium.org,posciak@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 688541 , 735959 > > Review-Url: https://codereview.chromium.org/2548443002 > Cr-Commit-Position: refs/heads/master@{#484874} > Committed: https://chromium.googlesource.com/chromium/src/+/829b1d57525c3c6549d18a2c85a96527d59ea5e9 TBR=emircan@chromium.org,magjed@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 688541 , 735959 Review-Url: https://codereview.chromium.org/2973253002 Cr-Commit-Position: refs/heads/master@{#484960} [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/BUILD.gn [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/media/gpu/rtc_video_encoder.cc [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/media/gpu/rtc_video_encoder.h [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/media/gpu/rtc_video_encoder_factory.cc [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/media/gpu/rtc_video_encoder_factory.h [modify] https://crrev.com/df6e5a5c7e7c665603f9619930a1d7106b55160d/content/renderer/media/gpu/rtc_video_encoder_unittest.cc
,
Jul 10 2017
niklase@ - can you link to the perf regressions?
,
Jul 10 2017
,
Jul 11 2017
niklase@ - the example you link to is a 70% _improvement_ in CPU usage, so it looks like the CL actually enables HW encoding. Some other changes are regressions and not improvements, like frame rate and 720p PSNR. I can reproduce locally, and I will take a closer look tomorrow and investigate what is happening.
,
Jul 11 2017
True, some more data points: fps is down (sligthly): https://chromeperf.appspot.com/report?sid=cc469cad65cd098cb0f5f659728d7c60aae72b388c3ee174dace0ca2b4ca2b98&rev=484877 Quality down: https://chromeperf.appspot.com/report?sid=80ae0fc265a4da221163c46fe66155c463efd3f5b0075c1c565bb5342007908f&rev=484876
,
Jul 12 2017
I investigated this a bit more. My CL disables HW encoding and uses SW encoding instead, like you initially said niklase@. I don't know why goog_encode_usage_percent goes down from that. Anyway, the reason that we use SW encoding with my CL is that we have an internal H264 encoder with Constrained baseline profile that will be in top of the SDP codec list. If the HW encoder is also listed as Constrained baseline profile, it will replace the SW codec, but if the profile is something else, like Baseline profile ( media::VideoCodecProfile::H264PROFILE_BASELINE) it will be listed as another codec further down the SDP priority list, and it will not be used. I don't know the best way to solve this. I think the iOS HW encoder is actually producing Constrained baseline profile and not Baseline profile, so maybe we should introduce media::VideoCodecProfile::H264PROFILE_CONSTRAINED_BASELINE and use that instead. Another solution would be to prioritize external codecs when generating the SDP codec list in WebRTC. We would then support both H264 Baseline (using HW) and H264 Constrained Baseline (using SW).
,
Jul 12 2017
The encode time values for HW encode aren't really reliable since it's an async OS call and not actual processing time.
,
Jul 12 2017
googEncodeUsagePercent values are pointless for HW encode. As far as I read through, it represents "the average processing time of a frame divided by the average time difference between captured frames"[0]. It is not related to CPU usage at all. It is basically a scale of goog_avg_encode_ms: 10ms/33ms for SW and 35ms/33ms for HW encode[1]. HW encode has async calls and IPC which overlaps between two frames. About 5-10ms of the time is actually HW encode and rest is posting tasks. I think we can safely ignore this metric. magjed@, here is the list of supported profiles for mac for instance[2]. HW encoders are supporting baseline and main/high can be added easily. I think prioritizing external codecs when generating the SDP codec list is the right solution. Should we list them in an ascending order then? [0] https://cs.chromium.org/chromium/src/third_party/webrtc/video/overuse_frame_detector.cc?rcl=a5b6c5225f63063a54130acc8da4c808c648e360&l=120 [1] https://chromeperf.appspot.com/report?sid=1d93bd935f62beed2239bf4be5290ed6667ddb53c5b47bc6084692d5a1c987ec&rev=484963 [2] https://github.com/BigHappy/MacOSX10.10.DK/blob/28e297bbd81263a8430474172975a4730dbbaecd/System/Library/Frameworks/VideoToolbox.framework/Versions/A/Headers/VTCompressionProperties.h#L234
,
Jul 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/external/webrtc.git/+/06f3aae345854ba9dcc5ae3b603de1f86505acf9 commit 06f3aae345854ba9dcc5ae3b603de1f86505acf9 Author: magjed <magjed@webrtc.org> Date: Fri Jul 14 17:36:23 2017 Prefer external video codecs over internal in SDP Currently, when we generate the list of supported video codecs that will be signaled in SDP, we start with the internal video codecs and then append the external video codecs. When we create a video encoder for a given codec, we prefer an external encoder over an internal encoder. This CL lists the external video codecs first in SDP instead, so that we consistently prefer external video codecs over internal. The reason for doing this is that we will otherwise prefer an internal SW H264 encoder over an external HW H264 encoder if the H264 profiles differs. BUG= chromium:688541 Review-Url: https://codereview.webrtc.org/2974383002 Cr-Commit-Position: refs/heads/master@{#19026} [modify] https://crrev.com/06f3aae345854ba9dcc5ae3b603de1f86505acf9/webrtc/media/engine/webrtcvideoengine.cc [modify] https://crrev.com/06f3aae345854ba9dcc5ae3b603de1f86505acf9/webrtc/media/engine/webrtcvideoengine_unittest.cc
,
Jul 17 2017
Manually reverted due to causing failure in upstreaming to Chromium https://codereview.webrtc.org/2982053002/
,
Jul 25 2017
The CL to prefer external codecs was reverted because it breaks the Android tests. The reason the Android tests failed with my CL is because they will then use the HW codecs instead of the SW codecs in the tests, and the HW codecs are crashing.
,
Jul 25 2017
Changing the default order of the codecs in SDP might be too big of a change, as it will change the default codec selection of e.g. Chrome. It would be much easier to update the perf tests that were failing to prefer HW H264 over SW H264. emircan@ - what do you think?
,
Jul 25 2017
cc braveyao@ who worked on Android to answer that. AFAIK there isn't even SW H264 encoder implementation on Android.
,
Jul 26 2017
Presently in chromium, HW 264 is the only option on Android and is enabled by default on supported devices according to: -whitelist https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java?type=cs&sq=package:chromium&l=477 -blacklist https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium/media/MediaCodecUtil.java?type=cs&sq=package:chromium&l=519
,
Aug 2 2017
To clarify; the Android tests that were failing didn't re-arrange the video codecs in SDP, they used the default. The default (first one) used to be SW VP8, and with my CL it changed to HW H264, so no SW H264 was involved on Android. Similarly, some Mac tests became flaky, because they also suddenly started to use HW H264 instead of SW VP8. Defaulting to HW codecs instead of SW codecs in WebRTC is kind of a big change and it will affect more than just tests, so I'm reluctant to take it on even though I think it's something we should do at some point. To move forward with supporting multiple H264 profiles in Chrome, I think it's easier to keep the HW codecs at the back of the SDP list and update the perf tests to be unaffected instead. One thing that will change when fixing this issue is that the HW H264 for Mac will be negotiated as Baseline profile instead of previous Constrained Baseline, but this is correct and intended behavior. The SW H264 will keep being listed as Constrained Baseline and have a separate payload type, so it's up to clients to select what codec they want. This is also intended behavior, but will probably affect some Chrome clients.
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d10d214a1783c08dd1fb887600e408d87f520e3 commit 4d10d214a1783c08dd1fb887600e408d87f520e3 Author: magjed <magjed@chromium.org> Date: Wed Aug 02 11:56:04 2017 Reland of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:1 of https://codereview.chromium.org/2973253002/ ) Reason for revert: Update test to still use HW version of H264. Original issue's description: > Revert of TCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:190001 of https://codereview.chromium.org/2548443002/ ) > > Reason for revert: > Reverting this since it's causing multiple perf regression on mac, looks like HW encode/decode might get disabled. > > Original issue's description: > > Reland of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #1 id:1 of https://codereview.chromium.org/2521923002/ ) > > > > Reason for revert: > > Try again. > > > > Original issue's description: > > > Revert of RTCVideoEncoder: Report H264 profile information to WebRTC (patchset #3 id:60001 of https://codereview.chromium.org/2499973002/ ) > > > > > > Reason for revert: > > > Causes these tests to fail on chromium.webrtc bots for Win and Mac: > > > WebRtcPerfBrowserTest.MANUAL_RunsAudioVideoCall60SecsAndLogsInternalMetricsH264 > > > WebRtcVideoQualityBrowserTests/WebRtcVideoQualityBrowserTest.MANUAL_TestVideoQualityH264 > > > https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/30367 > > > https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/62661 > > > > > > Original issue's description: > > > > RTCVideoEncoder: Report H264 profile information to WebRTC > > > > > > > > This CL updates RTCVideoEncoderFactory to report cricket::VideoCodecs > > > > instead of WebRtcVideoEncoderFactory::VideoCodecs. The H264 profile > > > > information is added to the cricket::VideoCodec so that WebRTC receives > > > > this information. Also, the mapping between media::VideoCodecProfiles > > > > and cricket::VideoCodecs is cached so that we can send the > > > > media::VideoCodecProfile to RTCVideoEncoder instead of having to deal > > > > with webrtc::VideoCodecType. > > > > > > > > BUG= webrtc:6337 > > > > > > > > Committed: https://crrev.com/510eddede44cb4b67c8f17fdd68cefb780a668c5 > > > > Cr-Commit-Position: refs/heads/master@{#433508} > > > > > > TBR=emircan@chromium.org,posciak@chromium.org > > > # Skipping CQ checks because original CL landed less than 1 days ago. > > > NOPRESUBMIT=true > > > NOTREECHECKS=true > > > NOTRY=true > > > BUG= webrtc:6337 > > > > > > Committed: https://crrev.com/c2564bc627cb950b124ac8e41bc5fd3187f7ad9c > > > Cr-Commit-Position: refs/heads/master@{#433828} > > > > TBR=emircan@chromium.org,posciak@chromium.org > > # Not skipping CQ checks because original CL landed more than 1 days ago. > > BUG= 688541 , 735959 > > > > Review-Url: https://codereview.chromium.org/2548443002 > > Cr-Commit-Position: refs/heads/master@{#484874} > > Committed: https://chromium.googlesource.com/chromium/src/+/829b1d57525c3c6549d18a2c85a96527d59ea5e9 > > TBR=emircan@chromium.org,magjed@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 688541 , 735959 > > Review-Url: https://codereview.chromium.org/2973253002 > Cr-Commit-Position: refs/heads/master@{#484960} > Committed: https://chromium.googlesource.com/chromium/src/+/df6e5a5c7e7c665603f9619930a1d7106b55160d TBR=emircan@chromium.org,niklase@chromium.org,phoglund@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 688541 , 735959 Review-Url: https://codereview.chromium.org/2985263002 Cr-Commit-Position: refs/heads/master@{#491338} [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_browsertest.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_browsertest_base.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_browsertest_base.h [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_internals_perf_browsertest.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/browser/media/webrtc/webrtc_video_quality_browsertest.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/test/data/webrtc/munge_sdp.js [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/chrome/test/data/webrtc/peerconnection.js [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/BUILD.gn [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/media/gpu/rtc_video_encoder.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/media/gpu/rtc_video_encoder.h [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/media/gpu/rtc_video_encoder_factory.cc [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/media/gpu/rtc_video_encoder_factory.h [modify] https://crrev.com/4d10d214a1783c08dd1fb887600e408d87f520e3/content/renderer/media/gpu/rtc_video_encoder_unittest.cc
,
Sep 29 2017
The CL seems to stick now, so I'm marking this as fixed. We still should add support for e.g. H264 High Profile, but I'll track that in a separate issue. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by magjed@chromium.org
, Feb 6 2017