Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935643002:
Templated AudioEncoderFactory (Closed)
|
Created:
3 years, 6 months ago by kwiberg-webrtc Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), peah-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, kwiberg-webrtc, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTemplated AudioEncoderFactory
No real encoder implements the correct API yet, so we're just testing
dummies.
BUG=webrtc:7823
Review-Url: https://codereview.webrtc.org/2935643002
Cr-Commit-Position: refs/heads/master@{#18637}
Committed: https://chromium.googlesource.com/external/webrtc/+/653158338e27faca4dabc9acfe9204bd1e2f2810
Patch Set 1 #Patch Set 2 : need std::move #Patch Set 3 : better docs #
Total comments: 7
Patch Set 4 : rebase #Patch Set 5 : argument names #Patch Set 6 : Don't use or require ConfigType::IsOk() #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : try ossu@'s suggestion #
Total comments: 15
Patch Set 9 : address review comments #Patch Set 10 : better docs #
Messages
Total messages: 42 (22 generated)
The CQ bit was checked by kwiberg@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 checked by kwiberg@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.
kwiberg@webrtc.org changed reviewers: + ossu@webrtc.org, solenberg@webrtc.org
lgtm https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:121: // AudioCodecInfo QueryAudioEncoder(const ConfigType&); missing param name https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:125: // std::unique_ptr<AudioEncoder> MakeAudioEncoder(const ConfigType&, here too https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:134: rtc::scoped_refptr<AudioEncoderFactory> CreateAudioEncoderFactory() { Could this go first in the file, above impl?
https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:121: // AudioCodecInfo QueryAudioEncoder(const ConfigType&); On 2017/06/13 14:56:14, the sun wrote: > missing param name Done. https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:125: // std::unique_ptr<AudioEncoder> MakeAudioEncoder(const ConfigType&, On 2017/06/13 14:56:14, the sun wrote: > here too Done. https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:134: rtc::scoped_refptr<AudioEncoderFactory> CreateAudioEncoderFactory() { On 2017/06/13 14:56:14, the sun wrote: > Could this go first in the file, above impl? No... maybe. It's subclassing AudioEncoderFactoryT<Ts...>, so we need the class (template) definition to be available. I could move this function into the _impl namespace and call it from another function that's defined first in the file, but then I'd have to forward declare that function even firster. And that forward declaration will be a pretty little thing like this: namespace audio_encoder_factory_template_impl { template <typename... Ts> rtc::scoped_refptr<AudioEncoderFactory> CreateAudioEncoderFactoryImpl(); } // namespace audio_encoder_factory_template_impl IOW, it would add 10-20 lines of code and make the code more complex and less pretty. I'm thinking it's a better trade-off to not do it.
Two new patch sets uploaded. The first adds the argument names that solenberg@ asked for; the second removes the arguably unnecessary IsOk() calls, so that the config types are no longer required to have such a method. Thoughts on whether that's a good idea?
https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:134: rtc::scoped_refptr<AudioEncoderFactory> CreateAudioEncoderFactory() { On 2017/06/13 19:28:35, kwiberg-webrtc wrote: > On 2017/06/13 14:56:14, the sun wrote: > > Could this go first in the file, above impl? > > No... maybe. It's subclassing AudioEncoderFactoryT<Ts...>, so we need the class > (template) definition to be available. I could move this function into the _impl > namespace and call it from another function that's defined first in the file, > but then I'd have to forward declare that function even firster. And that > forward declaration will be a pretty little thing like this: > > namespace audio_encoder_factory_template_impl { > template <typename... Ts> > rtc::scoped_refptr<AudioEncoderFactory> CreateAudioEncoderFactoryImpl(); > } // namespace audio_encoder_factory_template_impl > > IOW, it would add 10-20 lines of code and make the code more complex and less > pretty. I'm thinking it's a better trade-off to not do it. Agree. I can live without such beauty.
Description was changed from ========== Templated AudioEncoderFactory No real encoder implements the correct API yet, so we're just testing dummies. BUG=webrtc:5806 ========== to ========== Templated AudioEncoderFactory No real encoder implements the correct API yet, so we're just testing dummies. BUG=webrtc:7823 ==========
Neat! I'd prefer it if you could get rid of the Dummy argument to the function templates, though. From what I've seen, creating a helper template struct to put the code in is a more common way of solving this problem. https://codereview.webrtc.org/2935643002/diff/90001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/90001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:49: template <typename...> You can get rid of the Dummy argument by making a (different) helper template struct and parameterizing it instead, like so: template <typename Ts...> AudioEncoderFactoryHelper; template <typename T, typename Ts...> AudioEncoderFactoryHelper<T, Ts...> { static void DoAppendSupportedEncoder(std::vector<AudioCodecSpec>* specs); // etc. }; template <> AudioEncoderFactoryHelper<> { /* Here be the ending cases, doing nothing or returning nullptr, etc. */}; You only need the forward declaration before AudioEncoderFactoryT, I believe.
The CQ bit was checked by kwiberg@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.
New patch set posted. I don't have a preference between the old and the new version, but the new one is pretty much exactly what you proposed, so I hope you like it!
https://codereview.webrtc.org/2935643002/diff/90001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/90001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_encoder_factory_template.h:49: template <typename...> On 2017/06/15 13:57:59, ossu wrote: > You can get rid of the Dummy argument by making a (different) helper template > struct and parameterizing it instead, like so: > > template <typename Ts...> AudioEncoderFactoryHelper; > template <typename T, typename Ts...> AudioEncoderFactoryHelper<T, Ts...> { > static void DoAppendSupportedEncoder(std::vector<AudioCodecSpec>* specs); > // etc. > }; > template <> AudioEncoderFactoryHelper<> { /* Here be the ending cases, doing > nothing or returning nullptr, etc. */}; > > You only need the forward declaration before AudioEncoderFactoryT, I believe. Done.
I do! \o/ lgtm! (also, you should remove the extraneous namespace comment, below) https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:93: }; // namespace audio_encoder_factory_template_impl nit: It's just the class ending here, the namespace ends two lines down. :)
just a few comments! https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:63: RTC_CHECK(enc); Is this an expected runtime condition or not? If the encoder is hardware backed, we could run out of resources and should be able to handle that without crashing. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:75: static const std::vector<AudioCodecSpec> specs = [] { Caching the specs preclude dynamically changing the list of supported codecs in face of e.g. running out of resources (like hardware encoding). https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:93: }; // namespace audio_encoder_factory_template_impl bad comment https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:97: // Make an AudioEncoderFactory that can create instances of the given encoders. TODO: Point at builtin_audio_encoder_factory for an example of how it is used.
The CQ bit was checked by kwiberg@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/...
New patch set posted. I acceded to all demands. :-) https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:63: RTC_CHECK(enc); On 2017/06/16 09:18:06, the sun wrote: > Is this an expected runtime condition or not? If the encoder is hardware backed, > we could run out of resources and should be able to handle that without > crashing. It's a CHECK, so it's not an expected runtime condition---I intended it to catch bugs in codec implementations. But if failing to create an encoder that we thought we could create is a real-world case, I'll just remove the check. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:75: static const std::vector<AudioCodecSpec> specs = [] { On 2017/06/16 09:18:06, the sun wrote: > Caching the specs preclude dynamically changing the list of supported codecs in > face of e.g. running out of resources (like hardware encoding). Yes, I hadn't considered that. Fixed. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:93: }; // namespace audio_encoder_factory_template_impl On 2017/06/16 09:11:35, ossu wrote: > nit: It's just the class ending here, the namespace ends two lines down. :) Done. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:97: // Make an AudioEncoderFactory that can create instances of the given encoders. On 2017/06/16 09:18:06, the sun wrote: > TODO: Point at builtin_audio_encoder_factory for an example of how it is used. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:121: // create an AudioDecoder. I think we should mention that the order of the codecs determines how codecs are prioritized, with the first being the most preferred one.
New patch set posted. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:121: // create an AudioDecoder. On 2017/06/16 11:20:45, ossu wrote: > I think we should mention that the order of the codecs determines how codecs are > prioritized, with the first being the most preferred one. Oh, yeah, that's an excellent idea! Done.
I'll land this shortly, unless you have more comments?
I've a couple of comments, but I'm not sure they're enough to stop this landing, nor something we'll resolve right now. We can bicker about them in the weeks to come. :) https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:63: RTC_CHECK(enc); On 2017/06/16 09:18:06, the sun wrote: > Is this an expected runtime condition or not? If the encoder is hardware backed, > we could run out of resources and should be able to handle that without > crashing. Should this be handled as failing to create an AudioEncoder object, though? The outside code relies on the fact that if QueryAudioEncoder() says yes, we will be able to create that encoder. There are no fallbacks. https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:75: static const std::vector<AudioCodecSpec> specs = [] { On 2017/06/16 09:18:06, the sun wrote: > Caching the specs preclude dynamically changing the list of supported codecs in > face of e.g. running out of resources (like hardware encoding). I think this isn't the wrong place to handle that. At the point GetSupportedEncoders is called, its too early to say if we'll be able to use that hardware resource or not. The output from this is already cached by WebRtcVoiceEngine, for example. Building the list on-demand is probably not much more expensive than copying it and would allow the application developer to change the list of codecs dynamically in some specific cases. So... fine, I guess. :)
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2935643002/#ps170001 (title: "better docs")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_encoder_factory_template.h (right): https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:63: RTC_CHECK(enc); On 2017/06/16 13:36:53, ossu wrote: > On 2017/06/16 09:18:06, the sun wrote: > > Is this an expected runtime condition or not? If the encoder is hardware > backed, > > we could run out of resources and should be able to handle that without > > crashing. > > Should this be handled as failing to create an AudioEncoder object, though? The > outside code relies on the fact that if QueryAudioEncoder() says yes, we will be > able to create that encoder. There are no fallbacks. Well, in what other way could we possibly handle it? Hmm. By forcing all codecs to fall back to software implementations, I guess? https://codereview.webrtc.org/2935643002/diff/130001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_encoder_factory_template.h:75: static const std::vector<AudioCodecSpec> specs = [] { On 2017/06/16 13:36:53, ossu wrote: > On 2017/06/16 09:18:06, the sun wrote: > > Caching the specs preclude dynamically changing the list of supported codecs > in > > face of e.g. running out of resources (like hardware encoding). > > I think this isn't the wrong place to handle that. At the point > GetSupportedEncoders is called, its too early to say if we'll be able to use > that hardware resource or not. The output from this is already cached by > WebRtcVoiceEngine, for example. Building the list on-demand is probably not much > more expensive than copying it and would allow the application developer to > change the list of codecs dynamically in some specific cases. So... fine, I > guess. :) > Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/27017)
The CQ bit was checked by solenberg@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": 170001, "attempt_start_ts": 1497634617886040,
"parent_rev": "9fbbdc2fef109d60136a651348902a39ab578eb7", "commit_rev":
"653158338e27faca4dabc9acfe9204bd1e2f2810"}
Message was sent while issue was closed.
Description was changed from ========== Templated AudioEncoderFactory No real encoder implements the correct API yet, so we're just testing dummies. BUG=webrtc:7823 ========== to ========== Templated AudioEncoderFactory No real encoder implements the correct API yet, so we're just testing dummies. BUG=webrtc:7823 Review-Url: https://codereview.webrtc.org/2935643002 Cr-Commit-Position: refs/heads/master@{#18637} Committed: https://chromium.googlesource.com/external/webrtc/+/653158338e27faca4dabc9acf... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://chromium.googlesource.com/external/webrtc/+/653158338e27faca4dabc9acf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
