Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881713002:
Reland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder (Closed)
|
Created:
3 years, 7 months ago by Chandan Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, emircan+watch+mediarecorder_chromium.org, mcasas+mediarecorder_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder
Link to original CL: https://codereview.chromium.org/2854363002/
Original CL description:
This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead
of an AudioFifo, to avoid copying data needlessly.
The original CL was reverted due to failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzY3OGM3NGNmM2UxN2FhZWY5NTYyZWUxM2RmOGM4MDNlMWQwMTRmYzYM
The failure was caused as uninitialized memory was being read.
This reland CL zeroes out all channels of incoming audio bus if the deque is empty.
BUG=None
Review-Url: https://codereview.chromium.org/2881713002
Cr-Commit-Position: refs/heads/master@{#472836}
Committed: https://chromium.googlesource.com/chromium/src/+/8e3f09678fd2199649fdfde1bb9cc0ec55240309
Patch Set 1 #
Total comments: 2
Patch Set 2 : Zero out all channels of audio_bus #Messages
Total messages: 37 (23 generated)
The CQ bit was checked by c.padhi@samsung.com 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by c.padhi@samsung.com 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
c.padhi@samsung.com changed reviewers: + mcasas@chromium.org, miu@chromium.org
https://codereview.chromium.org/2881713002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/audio_track_recorder.cc (right): https://codereview.chromium.org/2881713002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/audio_track_recorder.cc:279: return 0.0; Ref: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... So, it looks like the last attempt failed because uninitialized memory was being read. For example: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_... The only thing I can think of that would cause this is here. Before returning 0.0, you should probably call: audio_bus->Zero();
The CQ bit was checked by c.padhi@samsung.com 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Reland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder Link to original CL: https://codereview.chromium.org/2854363002/ Original CL description: This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead of an AudioFifo, to avoid copying data needlessly. BUG=None ========== to ========== Reland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder Link to original CL: https://codereview.chromium.org/2854363002/ Original CL description: This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead of an AudioFifo, to avoid copying data needlessly. The original CL was reverted due to failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... The failure was caused as uninitialized memory was being read. This reland CL zeroes out all channels of incoming audio bus if the deque is empty. BUG=None ==========
PTAL @PS2. Thank you. https://codereview.chromium.org/2881713002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/audio_track_recorder.cc (right): https://codereview.chromium.org/2881713002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/audio_track_recorder.cc:279: return 0.0; On 2017/05/12 23:55:21, miu wrote: > Ref: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > So, it looks like the last attempt failed because uninitialized memory was being > read. For example: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_... > > The only thing I can think of that would cause this is here. Before returning > 0.0, you should probably call: > > audio_bus->Zero(); Done.
The CQ bit was checked by miu@chromium.org
lgtm
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
@mcasas, RS please. Thanks.
The CQ bit was checked by c.padhi@samsung.com 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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/18 11:58:14, Chandan wrote: > @mcasas, RS please. Thanks. RS lgtm
The CQ bit was checked by c.padhi@samsung.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495124124879970,
"parent_rev": "980a5271bd212db2ad7a050c3d49628aed42deb1", "commit_rev":
"8e3f09678fd2199649fdfde1bb9cc0ec55240309"}
Message was sent while issue was closed.
Description was changed from ========== Reland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder Link to original CL: https://codereview.chromium.org/2854363002/ Original CL description: This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead of an AudioFifo, to avoid copying data needlessly. The original CL was reverted due to failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... The failure was caused as uninitialized memory was being read. This reland CL zeroes out all channels of incoming audio bus if the deque is empty. BUG=None ========== to ========== Reland Replace AudioFifo with a deque of AudioBus in ATR::AudioEncoder Link to original CL: https://codereview.chromium.org/2854363002/ Original CL description: This CL uses a std::deque<std::unique_ptr<media::AudioBus>> instead of an AudioFifo, to avoid copying data needlessly. The original CL was reverted due to failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... The failure was caused as uninitialized memory was being read. This reland CL zeroes out all channels of incoming audio bus if the deque is empty. BUG=None Review-Url: https://codereview.chromium.org/2881713002 Cr-Commit-Position: refs/heads/master@{#472836} Committed: https://chromium.googlesource.com/chromium/src/+/8e3f09678fd2199649fdfde1bb9c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/8e3f09678fd2199649fdfde1bb9c...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2888043007/ by jbroman@chromium.org. The reason for reverting is: Suspected of causing content_unittests MediaRecorderHandlerTest.EncodeAudioFrames/2 to fail on Windows: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
Message was sent while issue was closed.
On 2017/05/18 at 19:41:04, jbroman wrote: > A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2888043007/ by jbroman@chromium.org. > > The reason for reverting is: Suspected of causing content_unittests MediaRecorderHandlerTest.EncodeAudioFrames/2 to fail on Windows: > > https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2.... (and by fail, I mean time out)
Message was sent while issue was closed.
On 2017/05/18 at 19:41:17, jbroman wrote: > On 2017/05/18 at 19:41:04, jbroman wrote: > > A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2888043007/ by jbroman@chromium.org. > > > > The reason for reverting is: Suspected of causing content_unittests MediaRecorderHandlerTest.EncodeAudioFrames/2 to fail on Windows: > > > > https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2.... > > (and by fail, I mean time out) This CL also caused the return of MSAN failures, see https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/bu...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 472836 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
