New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 688541 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Last visit 16 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Resolve the mismatch between WebRTC's default H264 profile choice for HW codecs

Project Member Reported by emir...@chromium.org, Feb 3 2017

Issue description

When 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
 
Yes, it's related. I fixed this issue in https://codereview.chromium.org/2499973002/, but I had to revert the CL because it made some H264 tests to fail, and I haven't had time to investigate since.
Owner: magjed@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by magjed@chromium.org, Jul 10 2017

niklase@ - can you link to the perf regressions?

Comment 7 by magjed@chromium.org, 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.

Comment 9 by magjed@chromium.org, 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).
The encode time values for HW encode aren't really reliable since it's an async OS call and not actual processing time.
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
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Cc: henrika@chromium.org
Manually reverted due to causing failure in upstreaming to Chromium https://codereview.webrtc.org/2982053002/
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.
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?
Cc: braveyao@chromium.org
cc braveyao@ who worked on Android to answer that. AFAIK there isn't even SW H264  encoder implementation on Android. 
Labels: -Pri-3 Pri-2
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.
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
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