Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499973002:
RTCVideoEncoder: Report H264 profile information to WebRTC (Closed)
|
Created:
4 years, 1 month ago by magjed_chromium Modified:
4 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRTCVideoEncoder: 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}
Patch Set 1 #
Total comments: 8
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : Add CHECK_EQ size checks for profiles and codecs. #
Messages
Total messages: 35 (24 generated)
The CQ bit was checked by magjed@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...
Description was changed from ========== RTCVideoEncoder: Keep H264 profile information BUG=webrtc:6337 ========== to ========== RTCVideoEncoder: Report H264 profile information to WebRTC BUG=webrtc:6337 ==========
Description was changed from ========== RTCVideoEncoder: Report H264 profile information to WebRTC BUG=webrtc:6337 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by magjed@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...
Patchset #1 (id:1) has been deleted
magjed@chromium.org changed reviewers: + emircan@chromium.org, posciak@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:668: if (profile_ >= media::VP8PROFILE_MIN && profile_ <= media::VP8PROFILE_MAX) { I think we can store just media::VideoCodec in Impl instead of profile, then we could simplify the conditions here. https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.h:44: RTCVideoEncoder(const media::VideoCodecProfile& profile, Nit: perhaps just pass by value, as this is an enum? https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.h:44: std::vector<media::VideoCodecProfile> profiles_; Perhaps std::pair<media::VCP, cricket::VC> instead?
The CQ bit was checked by magjed@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...
https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:668: if (profile_ >= media::VP8PROFILE_MIN && profile_ <= media::VP8PROFILE_MAX) { On 2016/11/17 07:30:05, Pawel Osciak wrote: > I think we can store just media::VideoCodec in Impl instead of profile, then we > could simplify the conditions here. Done. I kept it as a webrtc::VideoCodecType in Impl to simplify the conditions here and minimize the diff. https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.h:44: RTCVideoEncoder(const media::VideoCodecProfile& profile, On 2016/11/17 07:30:05, Pawel Osciak wrote: > Nit: perhaps just pass by value, as this is an enum? Done. https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.h:44: std::vector<media::VideoCodecProfile> profiles_; On 2016/11/17 07:30:05, Pawel Osciak wrote: > Perhaps std::pair<media::VCP, cricket::VC> instead? Yes, I would prefer this myself, but the problem is that we need to return std::vector<cricket::VC>& in supported_codecs(). So then we would need to store both std::vector<cricket::VC> and std::vector<std::pair<media::VCP, cricket::VC>>.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.h:44: std::vector<media::VideoCodecProfile> profiles_; On 2016/11/17 13:08:03, magjed_chromium wrote: > On 2016/11/17 07:30:05, Pawel Osciak wrote: > > Perhaps std::pair<media::VCP, cricket::VC> instead? > > Yes, I would prefer this myself, but the problem is that we need to return > std::vector<cricket::VC>& in supported_codecs(). So then we would need to store > both std::vector<cricket::VC> and std::vector<std::pair<media::VCP, > cricket::VC>>. Can you add CHECK_EQ(profiles_.size(), supported_codecs_.size()) then? I would prefer if their synchronization is enforced by code as well. https://codereview.chromium.org/2499973002/diff/40001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2499973002/diff/40001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:98: base::Optional<cricket::VideoCodec> codec = VEAToWebRTCCodec(profile); const
The CQ bit was checked by magjed@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...
https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.h (right): https://codereview.chromium.org/2499973002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.h:44: std::vector<media::VideoCodecProfile> profiles_; On 2016/11/17 19:02:15, emircan wrote: > On 2016/11/17 13:08:03, magjed_chromium wrote: > > On 2016/11/17 07:30:05, Pawel Osciak wrote: > > > Perhaps std::pair<media::VCP, cricket::VC> instead? > > > > Yes, I would prefer this myself, but the problem is that we need to return > > std::vector<cricket::VC>& in supported_codecs(). So then we would need to > store > > both std::vector<cricket::VC> and std::vector<std::pair<media::VCP, > > cricket::VC>>. > > Can you add CHECK_EQ(profiles_.size(), supported_codecs_.size()) then? I would > prefer if their synchronization is enforced by code as well. Done. https://codereview.chromium.org/2499973002/diff/40001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/2499973002/diff/40001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_factory.cc:98: base::Optional<cricket::VideoCodec> codec = VEAToWebRTCCodec(profile); On 2016/11/17 19:02:15, emircan wrote: > const I need to have it non-const in order to move it a few lines below. A cricket::VideoCodec contains heavy stuff like std::map<std::string, std::string> so I prefer to move it instead of copying.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
The CQ bit was checked by magjed@chromium.org
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": 60001, "attempt_start_ts": 1479718474135920,
"parent_rev": ["9af0542466fdd8cd0dc53cc84818d6d95a312e33", null], "commit_rev":
["5b1b22db9d8e681e25499255ac54d8faeb4fac8d", null]}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/510eddede44cb4b67c8f17fdd68cefb780a668c5 Cr-Commit-Position: refs/heads/master@{#433508}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2521923002/ by magjed@chromium.org. The reason for reverting is: 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
