Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903823002:
[chrome.displaySource] Implement LPCM audio encoder. (Closed)
|
Created:
4 years, 8 months ago by e_hakkinen Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[chrome.displaySource] Implement LPCM audio encoder.
This CL adds an implementation of Linear Pulse-Code Modulation (LPCM)
audio encoder for Wi-Fi Display.
This part of a Wi-Fi Display audo encoder patch series:
* https://codereview.chromium.org/1899943005/
Add a base class for audio encoders.
* https://codereview.chromium.org/1903823002/ <-- This CL
Implement LPCM audio encoder.
* https://codereview.chromium.org/1905243003/
Integrate audio encoders into media pipeline.
BUG=242107
Committed: https://crrev.com/73a57a8c149d08a9ea79807670b79a51e06ad696
Cr-Commit-Position: refs/heads/master@{#390614}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Member initialization, NOTREACHED #
Total comments: 3
Patch Set 3 : Comments #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. BUG=242107 ========== to ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. BUG=242107 ==========
Description was changed from ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. BUG=242107 ========== to ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. * https://codereview.chromium.org/1905243003/ Integrate audio encoders into media pipeline. BUG=242107 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
eero.hakkinen@intel.com changed reviewers: + alexander.shalamov@intel.com, asargent@chromium.org, mikhail.pozdnyakov@intel.com
PTAL.
https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... File extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc (right): https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc:72: : WiFiDisplayAudioEncoder(audio_codec), following to be initialized: const media::AudioBus* current_input_bus_; int64_t input_frames_in_; media::AudioParameters input_params_; int fifo_end_frame_; int64_t fifo_frames_out_; https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc:215: return LPCMAudioStreamDescriptor::SAMPLING_FREQUENCY_48K; default: Neverreached();
https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... File extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc (right): https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc:72: : WiFiDisplayAudioEncoder(audio_codec), On 2016/04/28 08:05:25, Mikhail wrote: > following to be initialized: > > const media::AudioBus* current_input_bus_; > int64_t input_frames_in_; > media::AudioParameters input_params_; > int fifo_end_frame_; > int64_t fifo_frames_out_; Done, except for input_params_ which should not need to be explicitly initialized as it has a default ctor. https://codereview.chromium.org/1903823002/diff/40001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc:215: return LPCMAudioStreamDescriptor::SAMPLING_FREQUENCY_48K; On 2016/04/28 08:05:25, Mikhail wrote: > default: > Neverreached(); Done. I also removed the cast so that the code is well defined.
lgtm
https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... File extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc (right): https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc:43: WiFiDisplayElementaryStreamInfo CreateElementaryStreamInfo() const override; Override refers to https://codereview.chromium.org/1911403003/diff/20001/extensions/renderer/api... (thus still in review).
lgtm https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... File extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder.h (right): https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder.h:29: static void CreateLPCM(const wds::AudioCodec& audio_codec, nit: this could use a comment, since it's not immediately obvious why something that returns a "WiFiDisplayAudioEncoder" via a callback is called "CreateLPCM" instead of eg "CreateAudioEncoder"
https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... File extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder.h (right): https://codereview.chromium.org/1903823002/diff/60001/extensions/renderer/api... extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder.h:29: static void CreateLPCM(const wds::AudioCodec& audio_codec, On 2016/04/28 21:19:13, Antony Sargent wrote: > nit: this could use a comment, since it's not immediately obvious why something > that returns a "WiFiDisplayAudioEncoder" via a callback is called "CreateLPCM" > instead of eg "CreateAudioEncoder" Done.
This is pending https://codereview.chromium.org/1911403003/ to be reviewed and landed. I will commit once that happens.
The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903823002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903823002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by eero.hakkinen@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from asargent@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/1903823002/#ps80001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1903823002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1903823002/80001
Message was sent while issue was closed.
Description was changed from ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. * https://codereview.chromium.org/1905243003/ Integrate audio encoders into media pipeline. BUG=242107 ========== to ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. * https://codereview.chromium.org/1905243003/ Integrate audio encoders into media pipeline. BUG=242107 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/73a57a8c149d08a9ea79807670b79a51e06ad696 Cr-Commit-Position: refs/heads/master@{#390614}
Message was sent while issue was closed.
Description was changed from ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. * https://codereview.chromium.org/1905243003/ Integrate audio encoders into media pipeline. BUG=242107 ========== to ========== [chrome.displaySource] Implement LPCM audio encoder. This CL adds an implementation of Linear Pulse-Code Modulation (LPCM) audio encoder for Wi-Fi Display. This part of a Wi-Fi Display audo encoder patch series: * https://codereview.chromium.org/1899943005/ Add a base class for audio encoders. * https://codereview.chromium.org/1903823002/ <-- This CL Implement LPCM audio encoder. * https://codereview.chromium.org/1905243003/ Integrate audio encoders into media pipeline. BUG=242107 Committed: https://crrev.com/73a57a8c149d08a9ea79807670b79a51e06ad696 Cr-Commit-Position: refs/heads/master@{#390614} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
