Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537183002:
Encode HRTF database using FLAC (Closed)
|
Created:
4 years ago by Raymond Toy Modified:
3 years, 8 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-api_chromium.org, hongchan Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEncode HRTF database using FLAC
Encoding the HRTF database using FLAC shrinks the database file from
245804 bytes to 145812 bytes (40% less space), while still keeping
exactly the same data.
BUG=76391, 669545
TEST=hrtf-database.html
Committed: https://crrev.com/625aefda371b1533682cf59b9332404a913651f4
Cr-Commit-Position: refs/heads/master@{#437026}
Patch Set 1 #Patch Set 2 : Add README to describe how to create the Composite file. #Patch Set 3 : Use bufferLoader instead of fetch. #Patch Set 4 : Use the new audit system #
Total comments: 2
Patch Set 5 : Use the actual platform/audio/resources files in the tests #Patch Set 6 : Remove unneeded files. #
Total comments: 1
Messages
Total messages: 36 (20 generated)
The CQ bit was checked by rtoy@chromium.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.chromium.or...
The CQ bit was checked by rtoy@chromium.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.chromium.or...
rtoy@chromium.org changed reviewers: + hongchan@chromium.org
PTAL
Description was changed from ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=669545 TEST=hrtf-database.html ========== to ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391 TEST=hrtf-database.html ==========
Did some rough timing by adding some LOG's in DecodeAudioFileData (in audio_decoder.cc) that decodes the HRTF audio file. With the original WAV file, it seems to take 4.73 ms (average of 4 separate runs). (Min = 3.58, max = 6.46) With the new FLAC file, we have an average of 5.25 ms (of 4). Min = 3.16, max = 7.00) So the FLAC file takes, roughly, 10% more time to decode. This is probably acceptable to reduce the file size by half. I am surprised it takes about 5 ms to decode 61440 frames of data at 44100 Hz (stereo)
Why do we need two copies of 'flac' files in two different places? https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js (right): https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js:27: alert('BufferLoader: unable to load buffer ' + index + ", url: " + loader.urlList[index]); Since you've got this working, can we revert this logging change so we can remove this file from the CL? We can still use this, but we don't want any more edit on this.
On 2016/12/02 17:59:40, hongchan wrote: > Why do we need two copies of 'flac' files in two different places? I'm pretty sure run-webkit-tests can't go out and reach files outside of the LayoutTests directory. I'm also guessing it would be bad to have such a critical file listed in the grd file and existing in the LayoutTests directory (if that's even possible). > > https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js (right): > > https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js:27: > alert('BufferLoader: unable to load buffer ' + index + ", url: " + > loader.urlList[index]); > Since you've got this working, can we revert this logging change so we can > remove this file from the CL? > > We can still use this, but we don't want any more edit on this.
On 2016/12/02 18:12:03, Raymond Toy wrote: > On 2016/12/02 17:59:40, hongchan wrote: > > Why do we need two copies of 'flac' files in two different places? > > I'm pretty sure run-webkit-tests can't go out and reach files outside of the > LayoutTests directory. I'm mistaken. It seems as if you can. Let me try this on the bots.... > > I'm also guessing it would be bad to have such a critical file listed in the grd > file and existing in the LayoutTests directory (if that's even possible). > > > > > > https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js > (right): > > > > > https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js:27: > > alert('BufferLoader: unable to load buffer ' + index + ", url: " + > > loader.urlList[index]); > > Since you've got this working, can we revert this logging change so we can > > remove this file from the CL? > > > > We can still use this, but we don't want any more edit on this.
https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js (right): https://codereview.chromium.org/2537183002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/buffer-loader.js:27: alert('BufferLoader: unable to load buffer ' + index + ", url: " + loader.urlList[index]); On 2016/12/02 17:59:40, hongchan wrote: > Since you've got this working, can we revert this logging change so we can > remove this file from the CL? > > We can still use this, but we don't want any more edit on this. I can, but it's a useful fix to make it slightly easier to find out what caused the problem. At least until fetch() works better for us at which point we can stop using this completely.
lgtm
Description was changed from ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391 TEST=hrtf-database.html ========== to ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391, 669545 TEST=hrtf-database.html ==========
The CQ bit was checked by rtoy@chromium.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.chromium.or...
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 rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org Link to the patchset: https://codereview.chromium.org/2537183002/#ps100001 (title: "Remove unneeded files.")
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...)
rtoy@chromium.org changed reviewers: + foolip@chromium.org
foolip: PTAL at blink_resources.grd.
The CQ bit was checked by foolip@chromium.org
lgtm
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": 100001, "attempt_start_ts": 1481126707143150,
"parent_rev": "2e3f7b55ce1bd27663169c1495330492bc9588e8", "commit_rev":
"2adf7492896f782ecf19398f04b1f86dbaa0f866"}
Message was sent while issue was closed.
Description was changed from ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391, 669545 TEST=hrtf-database.html ========== to ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391, 669545 TEST=hrtf-database.html ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391, 669545 TEST=hrtf-database.html ========== to ========== Encode HRTF database using FLAC Encoding the HRTF database using FLAC shrinks the database file from 245804 bytes to 145812 bytes (40% less space), while still keeping exactly the same data. BUG=76391, 669545 TEST=hrtf-database.html Committed: https://crrev.com/625aefda371b1533682cf59b9332404a913651f4 Cr-Commit-Position: refs/heads/master@{#437026} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/625aefda371b1533682cf59b9332404a913651f4 Cr-Commit-Position: refs/heads/master@{#437026}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2537183002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/blink_resources.grd (left): https://codereview.chromium.org/2537183002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/blink_resources.grd:53: <include name="IDR_AUDIO_SPATIALIZATION_COMPOSITE" file="../Source/platform/audio/resources/Composite.wav" type="BINDATA"/> Can the .wav file be deleted now?
Message was sent while issue was closed.
On 2017/04/17 01:39:02, Nico wrote: > https://codereview.chromium.org/2537183002/diff/100001/third_party/WebKit/pub... > File third_party/WebKit/public/blink_resources.grd (left): > > https://codereview.chromium.org/2537183002/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/blink_resources.grd:53: <include > name="IDR_AUDIO_SPATIALIZATION_COMPOSITE" > file="../Source/platform/audio/resources/Composite.wav" type="BINDATA"/> > Can the .wav file be deleted now? Yeah, we can remove that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
