Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3007433002:
Let CreateVideoDecoder take a cricket::VideoCodec. (Closed)
|
Created:
3 years, 4 months ago by kthelgason Modified:
3 years, 3 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLet CreateVideoDecoder take a cricket::VideoCodec.
This makes it possible for decoder factories to actually provide any
video codec, not just the ones WebRTC knows about. It also brings
the decoder factory interface more in line with that of the encoder
factory.
BUG=webrtc:8140
Review-Url: https://codereview.webrtc.org/3007433002
Cr-Commit-Position: refs/heads/master@{#19654}
Committed: https://chromium.googlesource.com/external/webrtc/+/ebd4f7988eb31d7c4c2a9cf2581c77a33a0ad6c4
Patch Set 1 #Patch Set 2 : Add build dep #Patch Set 3 : Remove unrelated change #
Total comments: 13
Patch Set 4 : Code review #Patch Set 5 : rebase #Patch Set 6 : return a value from unused function #
Depends on Patchset: Messages
Total messages: 46 (30 generated)
The CQ bit was checked by kthelgason@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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/28489)
The CQ bit was checked by kthelgason@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/...
kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org
https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:32: virtual webrtc::VideoDecoder* CreateVideoDecoder( I want to have as few methods as possible in this interface, and it's easy for an implementation to just ignore VideoDecoderParams, so I don't think we need to add this method that uses cricket::VideoCodec but no params. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:39: virtual webrtc::VideoDecoder* CreateVideoDecoderWithParams( Add an empty line between the methods. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:42: return CreateVideoDecoder(codec); We have to be careful with backwards-compatability and this function will actually throw away VideoDecoderParams now. We should call the other CreateVideoDecoderWithParams from here. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine.cc (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine.cc:2180: if ((*old_decoders)[i].type == type) { This caching looks like a problem as well. I think we need to base it on |codec|, otherwise if an external factory has many different custom codecs, they will all be mapped to Generic here. I think it would be enough to store cricket::VideoCodec instead of webrtc::VideoCodecType in AllocatedDecoder and compare the codecs with '=='. It's an independent change and probably best to fix it in a separate CL. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.h (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.h:29: const cricket::VideoCodec& codec) override; I think we should add the params here. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm:107: NSString *codecName = [NSString stringWithUTF8String:codec.name.c_str()]; I think we could simplify this whole function now by just doing: id<RTCVideoDecoder> decoder = [decoder_factory_ createDecoder:[RTCVideoCodecInfo initWithNativeVideoCodec:codec]]; return decoder ? new ObjCVideoDecoder(decoder) : nullptr;
The CQ bit was checked by kthelgason@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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/28331)
https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:32: virtual webrtc::VideoDecoder* CreateVideoDecoder( On 2017/08/24 08:59:38, magjed_webrtc wrote: > I want to have as few methods as possible in this interface, and it's easy for > an implementation to just ignore VideoDecoderParams, so I don't think we need to > add this method that uses cricket::VideoCodec but no params. I can see that. I was planning on moving callers over to this method in a separate CL but maybe it's better to just introduce it at that point. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:39: virtual webrtc::VideoDecoder* CreateVideoDecoderWithParams( On 2017/08/24 08:59:39, magjed_webrtc wrote: > Add an empty line between the methods. Done. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:42: return CreateVideoDecoder(codec); On 2017/08/24 08:59:39, magjed_webrtc wrote: > We have to be careful with backwards-compatability and this function will > actually throw away VideoDecoderParams now. We should call the other > CreateVideoDecoderWithParams from here. > Yeah, I guess subclasses can have overridden implementations that do something with this. Good point. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine.cc (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine.cc:2180: if ((*old_decoders)[i].type == type) { On 2017/08/24 08:59:39, magjed_webrtc wrote: > This caching looks like a problem as well. I think we need to base it on > |codec|, otherwise if an external factory has many different custom codecs, they > will all be mapped to Generic here. > > I think it would be enough to store cricket::VideoCodec instead of > webrtc::VideoCodecType in AllocatedDecoder and compare the codecs with '=='. > It's an independent change and probably best to fix it in a separate CL. I spotted this as well but decided to fix it in a separate CL. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.h (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.h:29: const cricket::VideoCodec& codec) override; On 2017/08/24 08:59:39, magjed_webrtc wrote: > I think we should add the params here. Ack. https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm:107: NSString *codecName = [NSString stringWithUTF8String:codec.name.c_str()]; On 2017/08/24 08:59:39, magjed_webrtc wrote: > I think we could simplify this whole function now by just doing: > id<RTCVideoDecoder> decoder = [decoder_factory_ createDecoder:[RTCVideoCodecInfo > initWithNativeVideoCodec:codec]]; > return decoder ? new ObjCVideoDecoder(decoder) : nullptr; Sure, we can do that, but that just moves the responsibility of making sure that the codec being asked for is supported by the decoder. I'd prefer to keep it like this for now, and if we decide to change it I think it's a separate change.
lgtm https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm (right): https://codereview.webrtc.org/3007433002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/VideoToolbox/objc_video_decoder_factory.mm:107: NSString *codecName = [NSString stringWithUTF8String:codec.name.c_str()]; On 2017/08/24 10:49:47, kthelgason wrote: > On 2017/08/24 08:59:39, magjed_webrtc wrote: > > I think we could simplify this whole function now by just doing: > > id<RTCVideoDecoder> decoder = [decoder_factory_ > createDecoder:[RTCVideoCodecInfo > > initWithNativeVideoCodec:codec]]; > > return decoder ? new ObjCVideoDecoder(decoder) : nullptr; > > Sure, we can do that, but that just moves the responsibility of making sure that > the codec being asked for is supported by the decoder. > I'd prefer to keep it like this for now, and if we decide to change it I think > it's a separate change. I think we should fix the C++ code in core WebRTC so that we don't try to create invalid decoders in the first place, but let's fix it in a separate change as you suggest.
The CQ bit was checked by kthelgason@webrtc.org
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: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/21810)
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007433002/#ps80001 (title: "rebase")
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: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/29076)
The CQ bit was checked by kthelgason@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 kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007433002/#ps100001 (title: "return a value from unused function")
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_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by kthelgason@webrtc.org
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_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/1...)
The CQ bit was checked by kthelgason@webrtc.org
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_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by kthelgason@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": 100001, "attempt_start_ts": 1504521073543440,
"parent_rev": "3c39c0137afa274d1d524b150b50304b38a2847b", "commit_rev":
"ebd4f7988eb31d7c4c2a9cf2581c77a33a0ad6c4"}
Message was sent while issue was closed.
Description was changed from ========== Let CreateVideoDecoder take a cricket::VideoCodec. This makes it possible for decoder factories to actually provide any video codec, not just the ones WebRTC knows about. It also brings the decoder factory interface more in line with that of the encoder factory. BUG=webrtc:8140 ========== to ========== Let CreateVideoDecoder take a cricket::VideoCodec. This makes it possible for decoder factories to actually provide any video codec, not just the ones WebRTC knows about. It also brings the decoder factory interface more in line with that of the encoder factory. BUG=webrtc:8140 Review-Url: https://codereview.webrtc.org/3007433002 Cr-Commit-Position: refs/heads/master@{#19654} Committed: https://chromium.googlesource.com/external/webrtc/+/ebd4f7988eb31d7c4c2a9cf25... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/ebd4f7988eb31d7c4c2a9cf25... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
