Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3007073002:
Add new video codec factories (Closed)
|
Created:
3 years, 3 months ago by magjed_webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, the sun, tterriberry_mozilla.com, kwiberg-webrtc, brandtr Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd new video codec factories
This CL adds interfaces for the new video codec factories and wires them
up in WebRtcVideoEngine. The default behavior is unmodified however, and
the new code is currently unused except for the tests.
A follow-up CL will be uploaded for exposing them in the
PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/.
BUG=webrtc:7925
R=andersc@webrtc.org, stefan@webrtc.org
Review-Url: https://codereview.webrtc.org/3007073002 .
Cr-Commit-Position: refs/heads/master@{#19828}
Committed: https://webrtc.googlesource.com/src/+/d4b0c0562323e24d3075b6831bafa62ea8bf32bd
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 11
Patch Set 3 : Rebase SdpVideoFormat #
Total comments: 2
Patch Set 4 : Remove legacy comment. #Patch Set 5 : . #Patch Set 6 : . #
Messages
Total messages: 49 (34 generated)
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/27363) android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27442)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API. BUG=webrtc:7925 ========== to ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/. BUG=webrtc:7925 ==========
Description was changed from ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/. BUG=webrtc:7925 ========== to ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. The default behavior is unmodified however, and the new code is currently unused except for the tests. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/. BUG=webrtc:7925 ==========
magjed@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan - please review webrtc/api/video_codecs. There is a design doc in https://docs.google.com/a/google.com/document/d/12AhwdOP6hGauggUVo-wReuFaJhTd... if you want to discuss something in more detail.
magjed@webrtc.org changed reviewers: + andersc@webrtc.org
Anders - please take a look.
https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_decoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:19: } // namespace cricket What do you think of the fit of VideoCodec in the cricket vs the webrtc namespace? https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_encoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_encoder_factory.h:46: // Returns extra information about how the provided codec would be encoded, What does "encoded" mean in this context? It sounds like you're talking about encoding a codec. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine.cc (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine.cc:162: private: No need for private here and at line 143, right? Same for the factory adapter below.
https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_decoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:34: virtual std::unique_ptr<VideoDecoder> CreateVideoDecoder( Do we also need a version of this method that takes a VideoDecoderParams argument?
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/27926) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24823) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/29106)
Patchset #3 (id:60001) has been deleted
https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_decoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:19: } // namespace cricket On 2017/09/06 12:40:42, stefan-webrtc wrote: > What do you think of the fit of VideoCodec in the cricket vs the webrtc > namespace? It's unfortunate. cricket::VideoCodec represents the codec in SDP and is very simple and just contains a name as a string and parameters as a string key/value map. I introduced a new struct SdpVideoFormat to carry this information instead so that we don't have to depend on cricket::VideoCodec. This is similar to how they did it for audio. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:34: virtual std::unique_ptr<VideoDecoder> CreateVideoDecoder( On 2017/09/06 15:00:06, andersc wrote: > Do we also need a version of this method that takes a VideoDecoderParams > argument? I will investigate a bit, but hopefully we don't need to add VideoDecoderParams here. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_encoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_encoder_factory.h:46: // Returns extra information about how the provided codec would be encoded, On 2017/09/06 12:40:42, stefan-webrtc wrote: > What does "encoded" mean in this context? It sounds like you're talking about > encoding a codec. I updated the sentence to be more clear. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine.cc (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine.cc:162: private: On 2017/09/06 12:40:42, stefan-webrtc wrote: > No need for private here and at line 143, right? Same for the factory adapter > below. Ops, thanks.
https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_decoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:19: } // namespace cricket On 2017/09/10 15:27:49, magjed_webrtc wrote: > On 2017/09/06 12:40:42, stefan-webrtc wrote: > > What do you think of the fit of VideoCodec in the cricket vs the webrtc > > namespace? > > It's unfortunate. cricket::VideoCodec represents the codec in SDP and is very > simple and just contains a name as a string and parameters as a string key/value > map. I introduced a new struct SdpVideoFormat to carry this information instead > so that we don't have to depend on cricket::VideoCodec. This is similar to how > they did it for audio. Pardon the tangent, but do the two of you think that the separate “cricket” namespace is useful? If we had just one namespace, outsiders would have only one WebRTC namespace to remember (likely “webrtc”), and we ourselves would almost never need to mention the namespace at all since all our stuff would be in the same namespace. My opinion is that the “rtc” namespace could be folded into “webrtc”, but I have much less personal experience with the “cricket” namespace.
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #3 (id:100001) has been deleted
Ping - please take a look again. All dependencies have been landed now. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... File webrtc/api/video_codecs/video_decoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:19: } // namespace cricket On 2017/09/10 19:00:21, kwiberg-webrtc wrote: > On 2017/09/10 15:27:49, magjed_webrtc wrote: > > On 2017/09/06 12:40:42, stefan-webrtc wrote: > > > What do you think of the fit of VideoCodec in the cricket vs the webrtc > > > namespace? > > > > It's unfortunate. cricket::VideoCodec represents the codec in SDP and is very > > simple and just contains a name as a string and parameters as a string > key/value > > map. I introduced a new struct SdpVideoFormat to carry this information > instead > > so that we don't have to depend on cricket::VideoCodec. This is similar to how > > they did it for audio. > > Pardon the tangent, but do the two of you think that the separate “cricket” > namespace is useful? If we had just one namespace, outsiders would have only one > WebRTC namespace to remember (likely “webrtc”), and we ourselves would almost > never need to mention the namespace at all since all our stuff would be in the > same namespace. > > My opinion is that the “rtc” namespace could be folded into “webrtc”, but I have > much less personal experience with the “cricket” namespace. It's still often a sign that we are mixing two code bases and probably making the dependency graph worse when we are using cricket in the webrtc namespace. It's an interesting discussion, let's file an issue or start an email thread. https://codereview.webrtc.org/3007073002/diff/40001/webrtc/api/video_codecs/v... webrtc/api/video_codecs/video_decoder_factory.h:34: virtual std::unique_ptr<VideoDecoder> CreateVideoDecoder( On 2017/09/10 15:27:49, magjed_webrtc wrote: > On 2017/09/06 15:00:06, andersc wrote: > > Do we also need a version of this method that takes a VideoDecoderParams > > argument? > > I will investigate a bit, but hopefully we don't need to add VideoDecoderParams > here. I looked it up and we don't need to add VideoDecoderParams.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
neat! lgtm
One nit, otherwise lgtm https://codereview.webrtc.org/3007073002/diff/120001/webrtc/api/video_codecs/... File webrtc/api/video_codecs/video_encoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/120001/webrtc/api/video_codecs/... webrtc/api/video_codecs/video_encoder_factory.h:35: // webrtc::ViEExternalCodec::RegisterExternalSendCodec. This no longer exists.
https://codereview.webrtc.org/3007073002/diff/120001/webrtc/api/video_codecs/... File webrtc/api/video_codecs/video_encoder_factory.h (right): https://codereview.webrtc.org/3007073002/diff/120001/webrtc/api/video_codecs/... webrtc/api/video_codecs/video_encoder_factory.h:35: // webrtc::ViEExternalCodec::RegisterExternalSendCodec. On 2017/09/13 12:39:18, stefan-webrtc wrote: > This no longer exists. Thanks, I removed mentioning it. It was a copy pasted comment from old code...
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from andersc@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007073002/#ps140001 (title: "Remove legacy comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1505323293895310,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"097de857a0e78924ceaca6a02625deb47bac503f"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1505323293895310,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"097de857a0e78924ceaca6a02625deb47bac503f"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1505323293895310,
"parent_rev": "7a5bcdffaab90a05bc1146b2b1ea71c004e54d71", "commit_rev":
"097de857a0e78924ceaca6a02625deb47bac503f"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
Description was changed from ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. The default behavior is unmodified however, and the new code is currently unused except for the tests. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/. BUG=webrtc:7925 ========== to ========== Add new video codec factories This CL adds interfaces for the new video codec factories and wires them up in WebRtcVideoEngine. The default behavior is unmodified however, and the new code is currently unused except for the tests. A follow-up CL will be uploaded for exposing them in the PeerConnectionFactory API: https://codereview.webrtc.org/3004353002/. BUG=webrtc:7925 R=andersc@webrtc.org, stefan@webrtc.org Review-Url: https://codereview.webrtc.org/3007073002 . Cr-Commit-Position: refs/heads/master@{#19828} Committed: https://webrtc.googlesource.com/src/+/d4b0c0562323e24d3075b6831bafa62ea8bf32bd ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) manually as d4b0c0562323e24d3075b6831bafa62ea8bf32bd (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
