Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1725)

Issue 2449993003: Replace WebRtcVideoEncoderFactory::VideoCodec with cricket::VideoCodec (Closed)

Created:
4 years, 1 month ago by magjed_webrtc
Modified:
4 years, 1 month ago
Reviewers:
kthelgason, hta-webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Replace WebRtcVideoEncoderFactory::VideoCodec with cricket::VideoCodec This CL introduces two new functions to the WebRtcVideoEncoderFactory interface based on cricket::VideoFormat instead of WebRtcVideoEncoderFactory::VideoCodec. The functions are: WebRtcVideoEncoderFactory::CreateVideoEncoder() and WebRtcVideoEncoderFactory::supported_codecs(). In order to make a smooth transition to the new interface, the old functions are kept, and default implementations are provided for both the old and new functions so that external clients can switch from the old to the new functions in peace. The default implementations will just convert between cricket::VideoFormat and WebRtcVideoEncoderFactory::VideoCodec. Once all external clients have updated their code, the plan is to remove the old functions and all default implementations to make WebRtcVideoEncoderFactory a pure interface again. BUG=webrtc:6402, webrtc:6337 Committed: https://crrev.com/1e45cc6ee0b368f7cb8b90acc7fe4c1699bb4288 Cr-Commit-Position: refs/heads/master@{#14826}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Extract IsCodecSupportedFunction #

Patch Set 3 : Compare only codec names, not VideoCodec::Matches #

Unified diffs Side-by-side diffs Delta from patch set Stats (+214 lines, -147 lines) Patch
M webrtc/api/android/jni/androidmediaencoder_jni.h View 1 chunk +8 lines, -4 lines 0 comments Download
M webrtc/api/android/jni/androidmediaencoder_jni.cc View 1 2 5 chunks +13 lines, -22 lines 0 comments Download
M webrtc/media/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/media/base/codec.h View 1 3 chunks +7 lines, -0 lines 0 comments Download
M webrtc/media/base/codec.cc View 1 2 2 chunks +27 lines, -1 line 0 comments Download
M webrtc/media/engine/fakewebrtcvideoengine.h View 1 4 chunks +7 lines, -13 lines 0 comments Download
M webrtc/media/engine/webrtcvideoencoderfactory.h View 3 chunks +35 lines, -9 lines 0 comments Download
A webrtc/media/engine/webrtcvideoencoderfactory.cc View 1 chunk +60 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.h View 2 chunks +0 lines, -3 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.cc View 1 12 chunks +35 lines, -74 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2_unittest.cc View 17 chunks +20 lines, -21 lines 0 comments Download
M webrtc/media/media.gyp View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 33 (24 generated)
magjed_webrtc
Please take a look.
4 years, 1 month ago (2016-10-26 15:29:32 UTC) #16
kthelgason
some small suggestions, otherwise lgtm https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc#newcode658 webrtc/media/engine/webrtcvideoengine2.cc:658: } This code exists ...
4 years, 1 month ago (2016-10-28 08:26:54 UTC) #19
magjed_webrtc
https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc#newcode658 webrtc/media/engine/webrtcvideoengine2.cc:658: } On 2016/10/28 08:26:54, kthelgason wrote: > This code ...
4 years, 1 month ago (2016-10-28 10:27:31 UTC) #21
hta-webrtc
lgtm I assume that all tests pass and that it works with Chrome... we should ...
4 years, 1 month ago (2016-10-28 11:13:11 UTC) #22
kthelgason
https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2449993003/diff/60001/webrtc/media/engine/webrtcvideoengine2.cc#newcode730 webrtc/media/engine/webrtcvideoengine2.cc:730: } On 2016/10/28 10:27:31, magjed_webrtc wrote: > On 2016/10/28 ...
4 years, 1 month ago (2016-10-28 13:19:45 UTC) #24
magjed_webrtc
I had to revert comparing codecs with VideoCodec::Matches and compare using codec names for now. ...
4 years, 1 month ago (2016-10-28 13:30:17 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2449993003/120001
4 years, 1 month ago (2016-10-28 14:42:14 UTC) #29
commit-bot: I haz the power
Committed patchset #3 (id:120001)
4 years, 1 month ago (2016-10-28 14:43:51 UTC) #31
commit-bot: I haz the power
4 years, 1 month ago (2016-10-28 14:44:01 UTC) #33
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/1e45cc6ee0b368f7cb8b90acc7fe4c1699bb4288
Cr-Commit-Position: refs/heads/master@{#14826}

Powered by Google App Engine
This is Rietveld 408576698