Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2185953002:
Add decoder-specific settings with proper lifetime. (Closed)
|
Created:
4 years, 4 months ago by johan Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd decoder-specific settings with proper lifetime.
Utilize these settings for h264 sprop-parameter-sets.
BUG=webrtc:5948
Committed: https://crrev.com/3859c89b2ed6935b7c92d7944fed309d54b10c5b
Cr-Commit-Position: refs/heads/master@{#13656}
Patch Set 1 #
Total comments: 5
Patch Set 2 : remove refcounting #
Total comments: 13
Patch Set 3 : coding style #Patch Set 4 : Function name begins upper case. #Patch Set 5 : fix unit test #
Total comments: 1
Patch Set 6 : Add h264_extra_settings to VideoReceiveStream::Decoder::ToString() #
Messages
Total messages: 30 (15 generated)
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h#newcode21 webrtc/config.h:21: #include "webrtc/base/scoped_ref_ptr.h" Not sure what the current policy is regarding importing utility classes into the base api, as doing that may result in changes to those utilities breaking external applications. Especially worried about scoped_ref_ptr. Would've been nice with more support for stl here :/ https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h#newcode164 webrtc/config.h:164: }; If we're using an Optional for each codec, I'm not sure we really need the refcounting (assuming copyable extras)? https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:2338: new rtc::RefCountedObject<webrtc::DecoderSpecificSettings>(); No need to create this instance if props not found below. https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:3621: // sprop = decoder_specifics->h264_extra_settings->spropParameterSets; Missing cleanup? :) https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:3623: cfg.decoders[0].decoder_specific->h264_extra_settings->spropParameterSets; h264_extra_settings is an optional, check if populated before dereferencing
On 2016/07/27 08:37:25, språng wrote: > https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h > File webrtc/config.h (right): > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h#newcode21 > webrtc/config.h:21: #include "webrtc/base/scoped_ref_ptr.h" > Not sure what the current policy is regarding importing utility classes into the > base api, as doing that may result in changes to those utilities breaking > external applications. Especially worried about scoped_ref_ptr. Would've been > nice with more support for stl here :/ Removed reference counting and scoped_ref_ptr as discussed. > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/config.h#newcode164 > webrtc/config.h:164: }; > If we're using an Optional for each codec, I'm not sure we really need the > refcounting (assuming copyable extras)? See above. > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2.cc:2338: new > rtc::RefCountedObject<webrtc::DecoderSpecificSettings>(); > No need to create this instance if props not found below. Removed. > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2_unittest.cc:3621: // sprop = > decoder_specifics->h264_extra_settings->spropParameterSets; > Missing cleanup? :) Well, yes. Fixed. > > https://codereview.webrtc.org/2185953002/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2_unittest.cc:3623: > cfg.decoders[0].decoder_specific->h264_extra_settings->spropParameterSets; > h264_extra_settings is an optional, check if populated before dereferencing Done.
Description was changed from ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG= ========== to ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG=webrtc:5948 ==========
sprang@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm +stefan ptal https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2337: cricket::CodecParameterMap::const_iterator found = nit: "auto" would be easier on the eye :)
https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode150 webrtc/config.h:150: struct VideoDecoderH264extraSettings { H264Extra Is it necessary to even mention "Extra"? :) https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode151 webrtc/config.h:151: std::string spropParameterSets; no camel case style https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode158 webrtc/config.h:158: // vp8_extra_settings, and vp9_extra_settings will be added when required remove this comment https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2334: void configureDecoderSpecifics(webrtc::VideoReceiveStream::Decoder* decoder, No camel case https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2335: const cricket::VideoCodec& recvVideoCodec) { same here https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2337: cricket::CodecParameterMap::const_iterator found = rename found it instead, that's what is typically used in the code.
PTAL https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode150 webrtc/config.h:150: struct VideoDecoderH264extraSettings { On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > H264Extra > > Is it necessary to even mention "Extra"? :) You have a point. ;-) https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode151 webrtc/config.h:151: std::string spropParameterSets; On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > no camel case style Done. https://codereview.webrtc.org/2185953002/diff/20001/webrtc/config.h#newcode158 webrtc/config.h:158: // vp8_extra_settings, and vp9_extra_settings will be added when required On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > remove this comment Done. https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2334: void configureDecoderSpecifics(webrtc::VideoReceiveStream::Decoder* decoder, On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > No camel case Style in this file seems to be camel case for function names, but starting upper case. Changed name to this style for consistence. See lines 150..367 for reference. https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2335: const cricket::VideoCodec& recvVideoCodec) { On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > same here Done. https://codereview.webrtc.org/2185953002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2337: cricket::CodecParameterMap::const_iterator found = On 2016/08/01 10:58:09, stefan-webrtc (holmer) wrote: > rename found it instead, that's what is typically used in the code. Went for "auto it".
lgtm
The CQ bit was checked by johan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2185953002/#ps60001 (title: "Function name begins upper case.")
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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/16393)
The CQ bit was checked by johan@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/...
johan@webrtc.org changed reviewers: + mflodman@webrtc.org
Adding mflodman as module owner for both webrtc/media/... and webrtc/config.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
One comment, LGTM with that addressed. https://codereview.webrtc.org/2185953002/diff/80001/webrtc/video_receive_stre... File webrtc/video_receive_stream.h (right): https://codereview.webrtc.org/2185953002/diff/80001/webrtc/video_receive_stre... webrtc/video_receive_stream.h:47: DecoderSpecificSettings decoder_specific; I'd like to add this to the ToString implementation in webrtc/video/video_receive_stream.cc. Not logging the string content, but indicating this has been set. In the same way as done for pointer configurations, e.g.: (pre_decode_callback ? "(EncodedFrameObserver)" : "nullptr");.
On 2016/08/04 13:20:59, mflodman wrote: > One comment, LGTM with that addressed. > > https://codereview.webrtc.org/2185953002/diff/80001/webrtc/video_receive_stre... > File webrtc/video_receive_stream.h (right): > > https://codereview.webrtc.org/2185953002/diff/80001/webrtc/video_receive_stre... > webrtc/video_receive_stream.h:47: DecoderSpecificSettings decoder_specific; > I'd like to add this to the ToString implementation in > webrtc/video/video_receive_stream.cc. Not logging the string content, but > indicating this has been set. In the same way as done for pointer > configurations, e.g.: > (pre_decode_callback ? "(EncodedFrameObserver)" : "nullptr");. mflodman ptal As DecoderSpecificSettings is a struct containing only rtc::Optional members (currently one), I propose adding h264_extra_settings to VideoReceiveStream::Decoder::ToString(). Using "nullptr" to indicate an rtc::Optional with no value, even if it is technical not exactly a nullptr (but behaves similar). Alternative implementation would creating DecoderSpecificSettings::ToString() method and calling this from VideoReceiveStream::Decoder::ToString().
LGTM
The CQ bit was checked by johan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2185953002/#ps100001 (title: "Add h264_extra_settings to VideoReceiveStream::Decoder::ToString()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG=webrtc:5948 ========== to ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG=webrtc:5948 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG=webrtc:5948 ========== to ========== Add decoder-specific settings with proper lifetime. Utilize these settings for h264 sprop-parameter-sets. BUG=webrtc:5948 Committed: https://crrev.com/3859c89b2ed6935b7c92d7944fed309d54b10c5b Cr-Commit-Position: refs/heads/master@{#13656} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3859c89b2ed6935b7c92d7944fed309d54b10c5b Cr-Commit-Position: refs/heads/master@{#13656} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
