Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655783004:
Decode entire in-memory file for WebAudio (Closed)
|
Created:
3 years, 11 months ago by Raymond Toy Modified:
3 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDecode entire in-memory file for WebAudio
DecodeAudioFileData assumed that any in-memory file to be decoded had
a known duration that could be estimated without reading the entire
file. For webm files produced by MediaRecorder, this can be false.
Read and decode the entire file, ignoring any estimated duration. This
will result in increased memory consumption because the final output
is built up in pieces before finally copying it to the final destination.
Add tests for decoding webm files and WAV files using float32 samples.
Vorbis tests need to be updated because the resulting outputs were
previously truncated.
BUG=673782
TEST=codec-tests/webm/webm-decode.html, codec-tests/wav/f32-44khz.html
Review-Url: https://codereview.chromium.org/2655783004
Cr-Commit-Position: refs/heads/master@{#449662}
Committed: https://chromium.googlesource.com/chromium/src/+/3489b2812bf4e670cc27627ee9a7a1fd1919f399
Patch Set 1 #Patch Set 2 : Implement streaming read and add test #Patch Set 3 : Clean up implementation. #
Total comments: 3
Patch Set 4 : WIP: always read entire file #Patch Set 5 : WIP: cleanup, add tests #
Total comments: 5
Patch Set 6 : WIP: update unit tests #Patch Set 7 : Remove old implmenetation and update tests #
Total comments: 31
Patch Set 8 : Address review comments #Patch Set 9 : Address review comments #
Total comments: 3
Patch Set 10 : Use int, not size_t. #Messages
Total messages: 58 (10 generated)
rtoy@chromium.org changed reviewers: + dalecurtis@chromium.org, hongchan@chromium.org
PTAL at this general approach. Instead of implementing a streaming reader, I implemented a method to read and discard the audio data, but returns the actual number of frames. This keeps most of the logic the same for the file reader. For the streaming reader, I couldn't think of any straightforward way of not having basically two copies of the decoded data in memory. (One to hold the streaming decoded data, and then a copy to the final destination_bus.)
Is this worth doing? It's extremely inefficient. I think instead you might want to change the interface such that you decode in chunks and queue into an AudioFIFO then copy it into a final bucket once all decoding is complete. As a bonus designing it this way will allow you to eventually switch to a streaming model just by swapping out the URLProtocol at a later date.
On 2017/01/24 22:47:13, DaleCurtis wrote: > Is this worth doing? It's extremely inefficient. I think instead you might want > to change the interface such that you decode in chunks and queue into an > AudioFIFO then copy it into a final bucket once all decoding is complete. We still have two copies in memory, though, right? This might be an issue for small devices. The old pre-Spitzer decoder had a copy of the encoded data in memory, one on disk, and a final decoded version in memory. (And if the duration was unknown, we kept growing a vector to hold the data, so we might have also had two copies in memory.) I don't think we had problems in the wild, but I did run out of memory (or disk) once in a long while while debugging. > > As a bonus designing it this way will allow you to eventually switch to a > streaming model just by swapping out the URLProtocol at a later date. Yeah, that's coming someday to webaudio, but how that works is still TBD and probably a year or two away.
On 2017/01/24 at 22:54:43, rtoy wrote: > On 2017/01/24 22:47:13, DaleCurtis wrote: > > Is this worth doing? It's extremely inefficient. I think instead you might want > > to change the interface such that you decode in chunks and queue into an > > AudioFIFO then copy it into a final bucket once all decoding is complete. > > We still have two copies in memory, though, right? > > This might be an issue for small devices. The old pre-Spitzer decoder had a copy of the encoded data in memory, one on disk, and a final decoded version in memory. (And if the duration was unknown, we kept growing a vector to hold the data, so we might have also had two copies in memory.) I don't think we had problems in the wild, but I did run out of memory (or disk) once in a long while while debugging. It's possible to design this such that memory usage is muxed_size + decoded_size + packet_size; versus muxed_size + 2 * decoded_size. To do this, you decode into a small fixed size buffer and copy into a container that can be expanded. At the least this implementation should decode and copy into a growing container to avoid to decode cycles.
On 2017/01/24 22:59:33, DaleCurtis wrote: > On 2017/01/24 at 22:54:43, rtoy wrote: > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > Is this worth doing? It's extremely inefficient. I think instead you might > want > > > to change the interface such that you decode in chunks and queue into an > > > AudioFIFO then copy it into a final bucket once all decoding is complete. > > > > We still have two copies in memory, though, right? > > > > This might be an issue for small devices. The old pre-Spitzer decoder had a > copy of the encoded data in memory, one on disk, and a final decoded version in > memory. (And if the duration was unknown, we kept growing a vector to hold the > data, so we might have also had two copies in memory.) I don't think we had > problems in the wild, but I did run out of memory (or disk) once in a long while > while debugging. > > It's possible to design this such that memory usage is muxed_size + decoded_size > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you decode into > a small fixed size buffer and copy into a container that can be expanded. At the > least this implementation should decode and copy into a growing container to > avoid to decode cycles. Don't know what kind of growing containers are available in chrome, but don't they typically grow by doubling the size of the container as needed which entails allocating memory and copying? If so, you still end up with about 2*decoded_size.
On 2017/01/24 at 23:03:02, rtoy wrote: > On 2017/01/24 22:59:33, DaleCurtis wrote: > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > Is this worth doing? It's extremely inefficient. I think instead you might > > want > > > > to change the interface such that you decode in chunks and queue into an > > > > AudioFIFO then copy it into a final bucket once all decoding is complete. > > > > > > We still have two copies in memory, though, right? > > > > > > This might be an issue for small devices. The old pre-Spitzer decoder had a > > copy of the encoded data in memory, one on disk, and a final decoded version in > > memory. (And if the duration was unknown, we kept growing a vector to hold the > > data, so we might have also had two copies in memory.) I don't think we had > > problems in the wild, but I did run out of memory (or disk) once in a long while > > while debugging. > > > > It's possible to design this such that memory usage is muxed_size + decoded_size > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you decode into > > a small fixed size buffer and copy into a container that can be expanded. At the > > least this implementation should decode and copy into a growing container to > > avoid to decode cycles. > > Don't know what kind of growing containers are available in chrome, but don't they typically grow by doubling the size of the container as needed which entails allocating memory and copying? If so, you still end up with about 2*decoded_size. That's a fair point, I think you could be smarter, but it may be difficult since WebAudio needs this data in a contiguous block. If you can avoid that it's fairly easy, just a list of chunks of variable size. Probably the most memory efficient method is to decode only the first couple packets and then just get a packet count for the remainder of the stream. I.e. demux it entirely w/o decoding 1 time through, then multiply by frame count average of first few frames. With that in hand allocate the memory you need and then redemux and decode.
On 2017/01/24 23:07:17, DaleCurtis wrote: > On 2017/01/24 at 23:03:02, rtoy wrote: > > On 2017/01/24 22:59:33, DaleCurtis wrote: > > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > > Is this worth doing? It's extremely inefficient. I think instead you > might > > > want > > > > > to change the interface such that you decode in chunks and queue into an > > > > > AudioFIFO then copy it into a final bucket once all decoding is > complete. > > > > > > > > We still have two copies in memory, though, right? > > > > > > > > This might be an issue for small devices. The old pre-Spitzer decoder had > a > > > copy of the encoded data in memory, one on disk, and a final decoded version > in > > > memory. (And if the duration was unknown, we kept growing a vector to hold > the > > > data, so we might have also had two copies in memory.) I don't think we had > > > problems in the wild, but I did run out of memory (or disk) once in a long > while > > > while debugging. > > > > > > It's possible to design this such that memory usage is muxed_size + > decoded_size > > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you decode > into > > > a small fixed size buffer and copy into a container that can be expanded. At > the > > > least this implementation should decode and copy into a growing container to > > > avoid to decode cycles. > > > > Don't know what kind of growing containers are available in chrome, but don't > they typically grow by doubling the size of the container as needed which > entails allocating memory and copying? If so, you still end up with about > 2*decoded_size. > > That's a fair point, I think you could be smarter, but it may be difficult since > WebAudio needs this data in a contiguous block. If you can avoid that it's > fairly easy, just a list of chunks of variable size. Probably the most memory > efficient method is to decode only the first couple packets and then just get a > packet count for the remainder of the stream. I.e. demux it entirely w/o > decoding 1 time through, then multiply by frame count average of first few > frames. With that in hand allocate the memory you need and then redemux and > decode. This is an interesting idea. For short files, I waste decoding twice, but for long files, I win. However, I can imagine that the first few packets could be silence where the packets would be quite small. Then the estimated number of frames would be way too large. But that might not be too bad. However, I am reminded of a bug where Android's media decoder sometimes returned huge durations (billions of years?) for perfectly valid mp3/aac files that were actually only a few seconds long.
On 2017/01/24 at 23:18:36, rtoy wrote: > On 2017/01/24 23:07:17, DaleCurtis wrote: > > On 2017/01/24 at 23:03:02, rtoy wrote: > > > On 2017/01/24 22:59:33, DaleCurtis wrote: > > > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > > > Is this worth doing? It's extremely inefficient. I think instead you > > might > > > > want > > > > > > to change the interface such that you decode in chunks and queue into an > > > > > > AudioFIFO then copy it into a final bucket once all decoding is > > complete. > > > > > > > > > > We still have two copies in memory, though, right? > > > > > > > > > > This might be an issue for small devices. The old pre-Spitzer decoder had > > a > > > > copy of the encoded data in memory, one on disk, and a final decoded version > > in > > > > memory. (And if the duration was unknown, we kept growing a vector to hold > > the > > > > data, so we might have also had two copies in memory.) I don't think we had > > > > problems in the wild, but I did run out of memory (or disk) once in a long > > while > > > > while debugging. > > > > > > > > It's possible to design this such that memory usage is muxed_size + > > decoded_size > > > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you decode > > into > > > > a small fixed size buffer and copy into a container that can be expanded. At > > the > > > > least this implementation should decode and copy into a growing container to > > > > avoid to decode cycles. > > > > > > Don't know what kind of growing containers are available in chrome, but don't > > they typically grow by doubling the size of the container as needed which > > entails allocating memory and copying? If so, you still end up with about > > 2*decoded_size. > > > > That's a fair point, I think you could be smarter, but it may be difficult since > > WebAudio needs this data in a contiguous block. If you can avoid that it's > > fairly easy, just a list of chunks of variable size. Probably the most memory > > efficient method is to decode only the first couple packets and then just get a > > packet count for the remainder of the stream. I.e. demux it entirely w/o > > decoding 1 time through, then multiply by frame count average of first few > > frames. With that in hand allocate the memory you need and then redemux and > > decode. > > > This is an interesting idea. For short files, I waste decoding twice, but for long files, I win. However, I can imagine that the first few packets could be silence where the packets would be quite small. Then the estimated number of frames would be way too large. But that might not be too bad. However, I am reminded of a bug where Android's media decoder sometimes returned huge durations (billions of years?) for perfectly valid mp3/aac files that were actually only a few seconds long. Well, I think you should keep the existing implementation when duration is known. Only get a frame count when duration is unknown. Packets are generally fixed size for audio decoding. I.e. aac always decodes to 1024 frame buffers, opus 4096 I think, etc.
On 2017/01/24 23:21:06, DaleCurtis wrote: > On 2017/01/24 at 23:18:36, rtoy wrote: > > On 2017/01/24 23:07:17, DaleCurtis wrote: > > > On 2017/01/24 at 23:03:02, rtoy wrote: > > > > On 2017/01/24 22:59:33, DaleCurtis wrote: > > > > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > > > > Is this worth doing? It's extremely inefficient. I think instead you > > > might > > > > > want > > > > > > > to change the interface such that you decode in chunks and queue > into an > > > > > > > AudioFIFO then copy it into a final bucket once all decoding is > > > complete. > > > > > > > > > > > > We still have two copies in memory, though, right? > > > > > > > > > > > > This might be an issue for small devices. The old pre-Spitzer decoder > had > > > a > > > > > copy of the encoded data in memory, one on disk, and a final decoded > version > > > in > > > > > memory. (And if the duration was unknown, we kept growing a vector to > hold > > > the > > > > > data, so we might have also had two copies in memory.) I don't think we > had > > > > > problems in the wild, but I did run out of memory (or disk) once in a > long > > > while > > > > > while debugging. > > > > > > > > > > It's possible to design this such that memory usage is muxed_size + > > > decoded_size > > > > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you > decode > > > into > > > > > a small fixed size buffer and copy into a container that can be > expanded. At > > > the > > > > > least this implementation should decode and copy into a growing > container to > > > > > avoid to decode cycles. > > > > > > > > Don't know what kind of growing containers are available in chrome, but > don't > > > they typically grow by doubling the size of the container as needed which > > > entails allocating memory and copying? If so, you still end up with about > > > 2*decoded_size. > > > > > > That's a fair point, I think you could be smarter, but it may be difficult > since > > > WebAudio needs this data in a contiguous block. If you can avoid that it's > > > fairly easy, just a list of chunks of variable size. Probably the most > memory > > > efficient method is to decode only the first couple packets and then just > get a > > > packet count for the remainder of the stream. I.e. demux it entirely w/o > > > decoding 1 time through, then multiply by frame count average of first few > > > frames. With that in hand allocate the memory you need and then redemux and > > > decode. > > > > > > This is an interesting idea. For short files, I waste decoding twice, but for > long files, I win. However, I can imagine that the first few packets could be > silence where the packets would be quite small. Then the estimated number of > frames would be way too large. But that might not be too bad. However, I am > reminded of a bug where Android's media decoder sometimes returned huge > durations (billions of years?) for perfectly valid mp3/aac files that were > actually only a few seconds long. > > Well, I think you should keep the existing implementation when duration is > known. Only get a frame count when duration is unknown. Packets are generally > fixed size for audio decoding. I.e. aac always decodes to 1024 frame buffers, > opus 4096 I think, etc. Yeah, for backward compatibility, we're keeping the old path, although there are some bugs complaining that the resulting data is too short because the estimated duration was too small. Is there an API to get the packet size? I really don't want to have to embed such knowledge in the decoder if possible.
On 2017/01/24 at 23:25:10, rtoy wrote: > On 2017/01/24 23:21:06, DaleCurtis wrote: > > On 2017/01/24 at 23:18:36, rtoy wrote: > > > On 2017/01/24 23:07:17, DaleCurtis wrote: > > > > On 2017/01/24 at 23:03:02, rtoy wrote: > > > > > On 2017/01/24 22:59:33, DaleCurtis wrote: > > > > > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > > > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > > > > > Is this worth doing? It's extremely inefficient. I think instead you > > > > might > > > > > > want > > > > > > > > to change the interface such that you decode in chunks and queue > > into an > > > > > > > > AudioFIFO then copy it into a final bucket once all decoding is > > > > complete. > > > > > > > > > > > > > > We still have two copies in memory, though, right? > > > > > > > > > > > > > > This might be an issue for small devices. The old pre-Spitzer decoder > > had > > > > a > > > > > > copy of the encoded data in memory, one on disk, and a final decoded > > version > > > > in > > > > > > memory. (And if the duration was unknown, we kept growing a vector to > > hold > > > > the > > > > > > data, so we might have also had two copies in memory.) I don't think we > > had > > > > > > problems in the wild, but I did run out of memory (or disk) once in a > > long > > > > while > > > > > > while debugging. > > > > > > > > > > > > It's possible to design this such that memory usage is muxed_size + > > > > decoded_size > > > > > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you > > decode > > > > into > > > > > > a small fixed size buffer and copy into a container that can be > > expanded. At > > > > the > > > > > > least this implementation should decode and copy into a growing > > container to > > > > > > avoid to decode cycles. > > > > > > > > > > Don't know what kind of growing containers are available in chrome, but > > don't > > > > they typically grow by doubling the size of the container as needed which > > > > entails allocating memory and copying? If so, you still end up with about > > > > 2*decoded_size. > > > > > > > > That's a fair point, I think you could be smarter, but it may be difficult > > since > > > > WebAudio needs this data in a contiguous block. If you can avoid that it's > > > > fairly easy, just a list of chunks of variable size. Probably the most > > memory > > > > efficient method is to decode only the first couple packets and then just > > get a > > > > packet count for the remainder of the stream. I.e. demux it entirely w/o > > > > decoding 1 time through, then multiply by frame count average of first few > > > > frames. With that in hand allocate the memory you need and then redemux and > > > > decode. > > > > > > > > > This is an interesting idea. For short files, I waste decoding twice, but for > > long files, I win. However, I can imagine that the first few packets could be > > silence where the packets would be quite small. Then the estimated number of > > frames would be way too large. But that might not be too bad. However, I am > > reminded of a bug where Android's media decoder sometimes returned huge > > durations (billions of years?) for perfectly valid mp3/aac files that were > > actually only a few seconds long. > > > > Well, I think you should keep the existing implementation when duration is > > known. Only get a frame count when duration is unknown. Packets are generally > > fixed size for audio decoding. I.e. aac always decodes to 1024 frame buffers, > > opus 4096 I think, etc. > > Yeah, for backward compatibility, we're keeping the old path, although there are some bugs complaining that the resulting data is too short because the estimated duration was too small. > > Is there an API to get the packet size? I really don't want to have to embed such knowledge in the decoder if possible. No, it's going to vary per codec. That's why I suggest decoding the first few packets to get an estimate of the output frame count then skip decoding and just multiply by total number of packets.
On 2017/01/24 23:59:31, DaleCurtis wrote: > On 2017/01/24 at 23:25:10, rtoy wrote: > > On 2017/01/24 23:21:06, DaleCurtis wrote: > > > On 2017/01/24 at 23:18:36, rtoy wrote: > > > > On 2017/01/24 23:07:17, DaleCurtis wrote: > > > > > On 2017/01/24 at 23:03:02, rtoy wrote: > > > > > > On 2017/01/24 22:59:33, DaleCurtis wrote: > > > > > > > On 2017/01/24 at 22:54:43, rtoy wrote: > > > > > > > > On 2017/01/24 22:47:13, DaleCurtis wrote: > > > > > > > > > Is this worth doing? It's extremely inefficient. I think instead > you > > > > > might > > > > > > > want > > > > > > > > > to change the interface such that you decode in chunks and queue > > > into an > > > > > > > > > AudioFIFO then copy it into a final bucket once all decoding is > > > > > complete. > > > > > > > > > > > > > > > > We still have two copies in memory, though, right? > > > > > > > > > > > > > > > > This might be an issue for small devices. The old pre-Spitzer > decoder > > > had > > > > > a > > > > > > > copy of the encoded data in memory, one on disk, and a final decoded > > > version > > > > > in > > > > > > > memory. (And if the duration was unknown, we kept growing a vector > to > > > hold > > > > > the > > > > > > > data, so we might have also had two copies in memory.) I don't > think we > > > had > > > > > > > problems in the wild, but I did run out of memory (or disk) once in > a > > > long > > > > > while > > > > > > > while debugging. > > > > > > > > > > > > > > It's possible to design this such that memory usage is muxed_size + > > > > > decoded_size > > > > > > > + packet_size; versus muxed_size + 2 * decoded_size. To do this, you > > > decode > > > > > into > > > > > > > a small fixed size buffer and copy into a container that can be > > > expanded. At > > > > > the > > > > > > > least this implementation should decode and copy into a growing > > > container to > > > > > > > avoid to decode cycles. > > > > > > > > > > > > Don't know what kind of growing containers are available in chrome, > but > > > don't > > > > > they typically grow by doubling the size of the container as needed > which > > > > > entails allocating memory and copying? If so, you still end up with > about > > > > > 2*decoded_size. > > > > > > > > > > That's a fair point, I think you could be smarter, but it may be > difficult > > > since > > > > > WebAudio needs this data in a contiguous block. If you can avoid that > it's > > > > > fairly easy, just a list of chunks of variable size. Probably the most > > > memory > > > > > efficient method is to decode only the first couple packets and then > just > > > get a > > > > > packet count for the remainder of the stream. I.e. demux it entirely w/o > > > > > decoding 1 time through, then multiply by frame count average of first > few > > > > > frames. With that in hand allocate the memory you need and then redemux > and > > > > > decode. > > > > > > > > > > > > This is an interesting idea. For short files, I waste decoding twice, but > for > > > long files, I win. However, I can imagine that the first few packets could > be > > > silence where the packets would be quite small. Then the estimated number of > > > frames would be way too large. But that might not be too bad. However, I > am > > > reminded of a bug where Android's media decoder sometimes returned huge > > > durations (billions of years?) for perfectly valid mp3/aac files that were > > > actually only a few seconds long. > > > > > > Well, I think you should keep the existing implementation when duration is > > > known. Only get a frame count when duration is unknown. Packets are > generally > > > fixed size for audio decoding. I.e. aac always decodes to 1024 frame > buffers, > > > opus 4096 I think, etc. > > > > Yeah, for backward compatibility, we're keeping the old path, although there > are some bugs complaining that the resulting data is too short because the > estimated duration was too small. > > > > Is there an API to get the packet size? I really don't want to have to embed > such knowledge in the decoder if possible. > > No, it's going to vary per codec. That's why I suggest decoding the first few > packets to get an estimate of the output frame count then skip decoding and just > multiply by total number of packets. How do I know how many packets are there? But never mind. After talking this over with hongchan@, we've decided to go for speed and take the memory hit. We'll do a streaming decoder. (I had actually implemented almost all of that but then switched to this idea.) Since we never got a bug report from people that couldn't decode big files on Android, we're guessing this double-memory usage isn't too severe.
Description was changed from ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Add a method to determine if the duration is known and a method to determine the actual number of frames in the file. Update DecodeAudioFileData to use these new methods to get the number of frames. This is only done if the duration is unknown so that we maintain backward compatibility with files whose duration can be estimated. BUG=673782 TEST= ========== to ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ==========
Description was changed from ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ========== to ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility\ with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ==========
Description was changed from ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility\ with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ========== to ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ==========
Dale, I've implemented the streaming reader. PTAL at the general approach.
I wouldn't call this the streaming approach, but it should work. The streaming approach is a streaming download from the site versus using an InMemoryURLProtocol. https://codereview.chromium.org/2655783004/diff/40001/content/renderer/media/... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/40001/content/renderer/media/... content/renderer/media/audio_decoder.cc:61: // Allocate and configure the output audio channel data. This adds an extra copy, how about returning the std::vector<> of AudioBus instead and doing the copy out of that? Or teaching AudioFileReader how to initialize a blink audio bus. https://codereview.chromium.org/2655783004/diff/40001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/40001/media/filters/audio_fil... media/filters/audio_file_reader.cc:250: std::unique_ptr<AudioBus> AudioFileReader::StreamingRead() { Needs refactor, so we don't have two copies of the decoding loop. https://codereview.chromium.org/2655783004/diff/40001/media/filters/audio_fil... media/filters/audio_file_reader.cc:327: if (codec_context_->sample_fmt == AV_SAMPLE_FMT_FLT) { I believe there's an AudioBus::FromInterleaved method for this now.
On 2017/01/25 22:42:25, DaleCurtis wrote: > I wouldn't call this the streaming approach, but it should work. The streaming > approach is a streaming download from the site versus using an > InMemoryURLProtocol. Yeah, it's not really streaming at all. I'll have to come up with a better name. I'll what I can do about refactoring all of the common bits and reducing the copying.
The layout test looks good. Are you going to address Dale's feedback?
On 2017/01/26 21:34:55, hongchan wrote: > The layout test looks good. Are you going to address Dale's feedback? Yes. I think I'm going to update Read to support reading into an audio_bus or destination_bus as appropriate.
On 2017/01/26 21:55:57, Raymond Toy wrote: > On 2017/01/26 21:34:55, hongchan wrote: > > The layout test looks good. Are you going to address Dale's feedback? > > Yes. I think I'm going to update Read to support reading into an audio_bus or > destination_bus as appropriate. In light of crbug.com/689344 and various other issues where the estimated duration doesn't match the actual duration, hongchan@ and I have decided that it's probably best to read in the entire file, ignoring the duration. This will break many things in small ways.
On 2017/02/07 at 20:04:29, rtoy wrote: > On 2017/01/26 21:55:57, Raymond Toy wrote: > > On 2017/01/26 21:34:55, hongchan wrote: > > > The layout test looks good. Are you going to address Dale's feedback? > > > > Yes. I think I'm going to update Read to support reading into an audio_bus or > > destination_bus as appropriate. > > In light of crbug.com/689344 and various other issues where the estimated duration doesn't match the actual duration, hongchan@ and I have decided that it's probably best to read in the entire file, ignoring the duration. > > This will break many things in small ways. Hmm, how is this different from what you were already planning and what we do today?
On 2017/02/07 20:51:52, DaleCurtis wrote: > On 2017/02/07 at 20:04:29, rtoy wrote: > > On 2017/01/26 21:55:57, Raymond Toy wrote: > > > On 2017/01/26 21:34:55, hongchan wrote: > > > > The layout test looks good. Are you going to address Dale's feedback? > > > > > > Yes. I think I'm going to update Read to support reading into an audio_bus > or > > > destination_bus as appropriate. > > > > In light of crbug.com/689344 and various other issues where the estimated > duration doesn't match the actual duration, hongchan@ and I have decided that > it's probably best to read in the entire file, ignoring the duration. > > > > This will break many things in small ways. > > Hmm, how is this different from what you were already planning and what we do > today? The original code used the duration always and failed if we didn't have an estimate. The code in the CL supported reading the entire file only if there was no duration. So the proposal here is always read the entire file, ignoring any duration estimate. This means some decodings will now be longer than before if the estimated duration was too short.
On 2017/02/07 at 21:34:12, rtoy wrote: > On 2017/02/07 20:51:52, DaleCurtis wrote: > > On 2017/02/07 at 20:04:29, rtoy wrote: > > > On 2017/01/26 21:55:57, Raymond Toy wrote: > > > > On 2017/01/26 21:34:55, hongchan wrote: > > > > > The layout test looks good. Are you going to address Dale's feedback? > > > > > > > > Yes. I think I'm going to update Read to support reading into an audio_bus > > or > > > > destination_bus as appropriate. > > > > > > In light of crbug.com/689344 and various other issues where the estimated > > duration doesn't match the actual duration, hongchan@ and I have decided that > > it's probably best to read in the entire file, ignoring the duration. > > > > > > This will break many things in small ways. > > > > Hmm, how is this different from what you were already planning and what we do > > today? > > The original code used the duration always and failed if we didn't have an estimate. The code in the CL supported reading the entire file only if there was no duration. > > So the proposal here is always read the entire file, ignoring any duration estimate. This means some decodings will now be longer than before if the estimated duration was too short. Gotcha, I forgot we clipped previously if the duration was wrong.
On 2017/02/07 21:48:58, DaleCurtis wrote: > On 2017/02/07 at 21:34:12, rtoy wrote: > > On 2017/02/07 20:51:52, DaleCurtis wrote: > > > On 2017/02/07 at 20:04:29, rtoy wrote: > > > > On 2017/01/26 21:55:57, Raymond Toy wrote: > > > > > On 2017/01/26 21:34:55, hongchan wrote: > > > > > > The layout test looks good. Are you going to address Dale's feedback? > > > > > > > > > > Yes. I think I'm going to update Read to support reading into an > audio_bus > > > or > > > > > destination_bus as appropriate. > > > > > > > > In light of crbug.com/689344 and various other issues where the estimated > > > duration doesn't match the actual duration, hongchan@ and I have decided > that > > > it's probably best to read in the entire file, ignoring the duration. > > > > > > > > This will break many things in small ways. > > > > > > Hmm, how is this different from what you were already planning and what we > do > > > today? > > > > The original code used the duration always and failed if we didn't have an > estimate. The code in the CL supported reading the entire file only if there > was no duration. > > > > So the proposal here is always read the entire file, ignoring any duration > estimate. This means some decodings will now be longer than before if the > estimated duration was too short. > > Gotcha, I forgot we clipped previously if the duration was wrong. On a slightly different topic: Is it a rule that audio_file_reader.cc can't use things from public/platform/WebAudioBus.h? When I try to, the code won't link, saying blink::WebAudioBus::channelData() can't be found.
Yes, AudioFileReader lives in media/filters whereas blink stuff is only available in media/blink; we could move it about some to change this, but there are some test clients of AudioFileReader.
On 2017/02/07 22:11:32, DaleCurtis wrote: > Yes, AudioFileReader lives in media/filters whereas blink stuff is only > available in media/blink; we could move it about some to change this, but there > are some test clients of AudioFileReader. Thanks. Not a big deal. I guess I'll have to return the vector of decoded packets from audio_file_reader so that audio_decoder can build up the destination_bus from the packets. Easy enough.
dalecurtis: Not quite ready for review, but I have a couple of questions on what should happen when we break of of the do-while loop to continue the big while loop. https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... media/filters/audio_file_reader.cc:283: break; This continues the while loop. Is that what we really want? https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... media/filters/audio_file_reader.cc:319: break; Is the expectation here that we'll just return whatever we have already decoded?
https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... media/filters/audio_file_reader.cc:283: break; On 2017/02/08 at 21:30:45, Raymond Toy wrote: > This continues the while loop. Is that what we really want? Up to you, but we traditionally ignore one-off audio decode errors since every audio packet is essentially a "keyframe" and decoding can continue smoothly. https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... media/filters/audio_file_reader.cc:319: break; On 2017/02/08 at 21:30:44, Raymond Toy wrote: > Is the expectation here that we'll just return whatever we have already decoded? Yes.
https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/80001/media/filters/audio_fil... media/filters/audio_file_reader.cc:283: break; On 2017/02/08 21:47:52, DaleCurtis wrote: > On 2017/02/08 at 21:30:45, Raymond Toy wrote: > > This continues the while loop. Is that what we really want? > > Up to you, but we traditionally ignore one-off audio decode errors since every > audio packet is essentially a "keyframe" and decoding can continue smoothly. Thanks. I'll add some comments to this effect so that I'll remember why.
dalecurtis: I'm updating some of the unit tests now and there are three failures. InfiniteDuration fails, but I assume that test is really for a file that didn't have an estimated duration. What should I do about that? Vorbis and WithVideo mostly pass once I changed the trimmed_frames to be the actual number of decoded frames. (Probably want to rename that parameter.) These tests, however, fail because the hash doesn't match. Can I just replace the expected hash with the actual hash?
On 2017/02/08 at 23:55:23, rtoy wrote: > dalecurtis: I'm updating some of the unit tests now and there are three failures. > > InfiniteDuration fails, but I assume that test is really for a file that didn't have an estimated duration. What should I do about that? > > Vorbis and WithVideo mostly pass once I changed the trimmed_frames to be the actual number of decoded frames. (Probably want to rename that parameter.) These tests, however, fail because the hash doesn't match. Can I just replace the expected hash with the actual hash? Yes to everything :)
On 2017/02/09 01:07:51, DaleCurtis wrote: > On 2017/02/08 at 23:55:23, rtoy wrote: > > dalecurtis: I'm updating some of the unit tests now and there are three > failures. > > > > InfiniteDuration fails, but I assume that test is really for a file that > didn't have an estimated duration. What should I do about that? > > > > Vorbis and WithVideo mostly pass once I changed the trimmed_frames to be the > actual number of decoded frames. (Probably want to rename that parameter.) > These tests, however, fail because the hash doesn't match. Can I just replace > the expected hash with the actual hash? > > Yes to everything :) Cool. I'm renaming InfiniteDuration to UnknownDuration and making it a normal RunTest, but skips the duration and frame checks. These work for me locally, but I'll let the bots try it out.
Description was changed from ========== Support webm file reader for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Implement a streaming read method that reads the encoded data and builds up the decoded audio data incrementally. This is only done if the duration is unknown so that we maintain backward compatibility with files whose duration can be estimated. BUG=673782 TEST=codec-tests/webm/webm-decode.html ========== to ========== Decode entire in-memory file for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Read and decode the entire file, ignoring any estimated duration. This will result in increased memory consumption because the final output is built up in pieces before finally copying it to the final destination. BUG=673782 TEST=codec-tests/webm/webm-decode.html ==========
Description was changed from ========== Decode entire in-memory file for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Read and decode the entire file, ignoring any estimated duration. This will result in increased memory consumption because the final output is built up in pieces before finally copying it to the final destination. BUG=673782 TEST=codec-tests/webm/webm-decode.html ========== to ========== Decode entire in-memory file for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Read and decode the entire file, ignoring any estimated duration. This will result in increased memory consumption because the final output is built up in pieces before finally copying it to the final destination. Add tests for decoding webm files and WAV files using float32 samples. Vorbis tests need to be updated because the resulting outputs were previously truncated. BUG=673782 TEST=codec-tests/webm/webm-decode.html, codec-tests/wav/f32-44khz.html ==========
PTAL at ps#7
https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:9: #include <iomanip> ? https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:58: std::vector<std::unique_ptr<AudioBus>> decodedAudioPackets; decoded_audio_packets. https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:59: int number_of_frames = reader.Read(decodedAudioPackets); Should be passing a pointer in? https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:70: for (auto&& packet : decodedAudioPackets) { && is not allowed per style guide. https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:72: for (size_t ch = 0; ch < number_of_channels; ++ch) { Just use src->CopyPartialFramesTo(...) https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:135: std::vector<std::unique_ptr<AudioBus>>& decodedAudioPackets) { decoded_audio_packets. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:206: std::unique_ptr<AudioBus> audio_bus = Does emplace_back() work here? https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.h (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.h:54: int Read(std::vector<std::unique_ptr<AudioBus>>& decodedAudioPackets); Pass by pointer instead of ref. decoded_audio_packets. I think using a list here might be better to avoid doubling behavior that comes with the vector. I don't think there's any benefit for the colocation of elements that the vector provides in this case. Don't have any data to back that last statement up though, so up to you. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:43: for (auto&& packet : decodedAudioPackets) { no && https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:110: EXPECT_EQ(reader_->HasKnownDuration(), false); If this is only used for testing now rename it as HasKnownDurationForTesting()
https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:9: #include <iomanip> On 2017/02/09 18:47:36, DaleCurtis wrote: > ? So I can std::fixed and std::setprecision so more digits are printed for the duration. Not really necessary, but for the tests that I ran, ms accuracy isn't really enough. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.h (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.h:54: int Read(std::vector<std::unique_ptr<AudioBus>>& decodedAudioPackets); On 2017/02/09 18:47:36, DaleCurtis wrote: > Pass by pointer instead of ref. decoded_audio_packets. I think using a list here > might be better to avoid doubling behavior that comes with the vector. I don't > think there's any benefit for the colocation of elements that the vector > provides in this case. > > Don't have any data to back that last statement up though, so up to you. Good point. I expect the size of this vector to be quite small compared to the actual decoded audio data.
https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:110: EXPECT_EQ(reader_->HasKnownDuration(), false); On 2017/02/09 18:47:36, DaleCurtis wrote: > If this is only used for testing now rename it as HasKnownDurationForTesting() It's used in audio_decoder.cc in a DVLOG. I'm debating on whether it's really needed now that we read the entire file. Somewhat useful for debugging, though.
https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:206: std::unique_ptr<AudioBus> audio_bus = On 2017/02/09 18:47:36, DaleCurtis wrote: > Does emplace_back() work here? You mean to emplace_back() this audio_bus onto decoded_audio_packets? Then I'd have to keep a pointer to this for further processing.
https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:206: std::unique_ptr<AudioBus> audio_bus = On 2017/02/09 at 19:59:29, Raymond Toy wrote: > On 2017/02/09 18:47:36, DaleCurtis wrote: > > Does emplace_back() work here? > > You mean to emplace_back() this audio_bus onto decoded_audio_packets? Then I'd have to keep a pointer to this for further processing. Yes, you'd just have a local raw ptr.
https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:58: std::vector<std::unique_ptr<AudioBus>> decodedAudioPackets; On 2017/02/09 18:47:36, DaleCurtis wrote: > decoded_audio_packets. Done. https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:59: int number_of_frames = reader.Read(decodedAudioPackets); On 2017/02/09 18:47:36, DaleCurtis wrote: > Should be passing a pointer in? Done. https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:70: for (auto&& packet : decodedAudioPackets) { On 2017/02/09 18:47:36, DaleCurtis wrote: > && is not allowed per style guide. Done. https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:72: for (size_t ch = 0; ch < number_of_channels; ++ch) { On 2017/02/09 18:47:36, DaleCurtis wrote: > Just use src->CopyPartialFramesTo(...) Unfortunately, destination_bus is not an AudioBus. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:135: std::vector<std::unique_ptr<AudioBus>>& decodedAudioPackets) { On 2017/02/09 18:47:36, DaleCurtis wrote: > decoded_audio_packets. Done. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.cc:206: std::unique_ptr<AudioBus> audio_bus = On 2017/02/09 20:29:38, DaleCurtis wrote: > On 2017/02/09 at 19:59:29, Raymond Toy wrote: > > On 2017/02/09 18:47:36, DaleCurtis wrote: > > > Does emplace_back() work here? > > > > You mean to emplace_back() this audio_bus onto decoded_audio_packets? Then > I'd have to keep a pointer to this for further processing. > > Yes, you'd just have a local raw ptr. Done. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader.h (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader.h:54: int Read(std::vector<std::unique_ptr<AudioBus>>& decodedAudioPackets); On 2017/02/09 18:47:36, DaleCurtis wrote: > Pass by pointer instead of ref. decoded_audio_packets. I think using a list here > might be better to avoid doubling behavior that comes with the vector. I don't > think there's any benefit for the colocation of elements that the vector > provides in this case. > > Don't have any data to back that last statement up though, so up to you. Passing by pointer. Decided to keep a vector because it's pretty simple. I think a vector of 1000 elements would hold about a million audio frames, so it's pretty small in comparison. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:43: for (auto&& packet : decodedAudioPackets) { On 2017/02/09 18:47:36, DaleCurtis wrote: > no && Done. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:110: EXPECT_EQ(reader_->HasKnownDuration(), false); On 2017/02/09 19:56:05, Raymond Toy wrote: > On 2017/02/09 18:47:36, DaleCurtis wrote: > > If this is only used for testing now rename it as HasKnownDurationForTesting() > > It's used in audio_decoder.cc in a DVLOG. I'm debating on whether it's really > needed now that we read the entire file. Somewhat useful for debugging, though. Renamed to HasKnownDurationForTesting. Still used in DVLOG which causes a presubmit warning, but won't block commits. Maybe I should just remove it from the DVLOG? https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:151: RunTest("bear-320x240-live.webm", Presubmit keeps telling me I need to reformat this file because of this and the two other tests below. Don't know if this will block the commit or not.
https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:9: #include <iomanip> On 2017/02/09 at 18:56:12, Raymond Toy wrote: > On 2017/02/09 18:47:36, DaleCurtis wrote: > > ? > > So I can std::fixed and std::setprecision so more digits are printed for the duration. Not really necessary, but for the tests that I ran, ms accuracy isn't really enough. Instead how about using TimeDelta::FromSecondsD().InMicroseconds? https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:110: EXPECT_EQ(reader_->HasKnownDuration(), false); On 2017/02/09 at 21:45:06, Raymond Toy wrote: > On 2017/02/09 19:56:05, Raymond Toy wrote: > > On 2017/02/09 18:47:36, DaleCurtis wrote: > > > If this is only used for testing now rename it as HasKnownDurationForTesting() > > > > It's used in audio_decoder.cc in a DVLOG. I'm debating on whether it's really > > needed now that we read the entire file. Somewhat useful for debugging, though. > > Renamed to HasKnownDurationForTesting. Still used in DVLOG which causes a presubmit warning, but won't block commits. Maybe I should just remove it from the DVLOG? Yeah either drop from DVLOG or rename back to what you had. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:151: RunTest("bear-320x240-live.webm", On 2017/02/09 at 21:45:07, Raymond Toy wrote: > Presubmit keeps telling me I need to reformat this file because of this and the two other tests below. > > Don't know if this will block the commit or not. Just run git cl format.
https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... File content/renderer/media/audio_decoder.cc (right): https://codereview.chromium.org/2655783004/diff/120001/content/renderer/media... content/renderer/media/audio_decoder.cc:9: #include <iomanip> On 2017/02/09 22:42:20, DaleCurtis wrote: > On 2017/02/09 at 18:56:12, Raymond Toy wrote: > > On 2017/02/09 18:47:36, DaleCurtis wrote: > > > ? > > > > So I can std::fixed and std::setprecision so more digits are printed for the > duration. Not really necessary, but for the tests that I ran, ms accuracy isn't > really enough. > > Instead how about using TimeDelta::FromSecondsD().InMicroseconds? That produces the result in microseconds, which makes it hard to read. I've decided to not to do anything since we print out the sample rate and the number of frames. If someone really cares, he can compute the duration accurately himself. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... File media/filters/audio_file_reader_unittest.cc (right): https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:110: EXPECT_EQ(reader_->HasKnownDuration(), false); On 2017/02/09 22:42:20, DaleCurtis wrote: > On 2017/02/09 at 21:45:06, Raymond Toy wrote: > > On 2017/02/09 19:56:05, Raymond Toy wrote: > > > On 2017/02/09 18:47:36, DaleCurtis wrote: > > > > If this is only used for testing now rename it as > HasKnownDurationForTesting() > > > > > > It's used in audio_decoder.cc in a DVLOG. I'm debating on whether it's > really > > > needed now that we read the entire file. Somewhat useful for debugging, > though. > > > > Renamed to HasKnownDurationForTesting. Still used in DVLOG which causes a > presubmit warning, but won't block commits. Maybe I should just remove it from > the DVLOG? > > Yeah either drop from DVLOG or rename back to what you had. Renamed. https://codereview.chromium.org/2655783004/diff/120001/media/filters/audio_fi... media/filters/audio_file_reader_unittest.cc:151: RunTest("bear-320x240-live.webm", On 2017/02/09 22:42:20, DaleCurtis wrote: > On 2017/02/09 at 21:45:07, Raymond Toy wrote: > > Presubmit keeps telling me I need to reformat this file because of this and > the two other tests below. > > > > Don't know if this will block the commit or not. > > Just run git cl format. Done.
lgtm, nice work! Did you see how this performs on http://www.scirra.com/labs/bugs/webaudiodecode/ from http://crbug.com/424174 ? https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... media/filters/audio_file_reader.cc:144: size_t total_frames = 0; Int? https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... media/filters/audio_file_reader.cc:210: if (codec_context_->sample_fmt == AV_SAMPLE_FMT_FLT) { Note, if memory balloons too much here we can store the AVFrames as AudioBuffers, see what is done in FFmpegAudioDecoder. This will keep the buffers in their native non-float sample format until they need to be converted into the AudioBus.
On 2017/02/09 23:24:43, DaleCurtis wrote: > lgtm, nice work! Did you see how this performs on > http://www.scirra.com/labs/bugs/webaudiodecode/ from http://crbug.com/424174 ? Wasn't that taken care of already when you switched Android to ffmpeg? > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > File media/filters/audio_file_reader.cc (right): > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > media/filters/audio_file_reader.cc:144: size_t total_frames = 0; > Int? > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > media/filters/audio_file_reader.cc:210: if (codec_context_->sample_fmt == > AV_SAMPLE_FMT_FLT) { > Note, if memory balloons too much here we can store the AVFrames as > AudioBuffers, see what is done in FFmpegAudioDecoder. This will keep the buffers > in their native non-float sample format until they need to be converted into the > AudioBus. I'll take a look. This is probably something we should do since it keeps the packets half as big.
https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... media/filters/audio_file_reader.cc:144: size_t total_frames = 0; On 2017/02/09 23:24:43, DaleCurtis wrote: > Int? Done.
On 2017/02/09 23:32:46, Raymond Toy wrote: > On 2017/02/09 23:24:43, DaleCurtis wrote: > > lgtm, nice work! Did you see how this performs on > > http://www.scirra.com/labs/bugs/webaudiodecode/ from http://crbug.com/424174 > ? > > Wasn't that taken care of already when you switched Android to ffmpeg? > > > > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > > File media/filters/audio_file_reader.cc (right): > > > > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > > media/filters/audio_file_reader.cc:144: size_t total_frames = 0; > > Int? > > > > > https://codereview.chromium.org/2655783004/diff/160001/media/filters/audio_fi... > > media/filters/audio_file_reader.cc:210: if (codec_context_->sample_fmt == > > AV_SAMPLE_FMT_FLT) { > > Note, if memory balloons too much here we can store the AVFrames as > > AudioBuffers, see what is done in FFmpegAudioDecoder. This will keep the > buffers > > in their native non-float sample format until they need to be converted into > the > > AudioBus. > > I'll take a look. This is probably something we should do since it keeps the > packets half as big. I think I'll do that in a separate CL.
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2655783004/#ps180001 (title: "Use int, not size_t.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/09 at 23:32:46, rtoy wrote: > On 2017/02/09 23:24:43, DaleCurtis wrote: > > lgtm, nice work! Did you see how this performs on > > http://www.scirra.com/labs/bugs/webaudiodecode/ from http://crbug.com/424174 ? > > Wasn't that taken care of already when you switched Android to ffmpeg? Yes, but this approach doubles memory complexity so I was curious if it runs any slower/faster, etc.
On 2017/02/10 17:48:34, DaleCurtis wrote: > On 2017/02/09 at 23:32:46, rtoy wrote: > > On 2017/02/09 23:24:43, DaleCurtis wrote: > > > lgtm, nice work! Did you see how this performs on > > > http://www.scirra.com/labs/bugs/webaudiodecode/ from > http://crbug.com/424174 ? > > > > Wasn't that taken care of already when you switched Android to ffmpeg? > > Yes, but this approach doubles memory complexity so I was curious if it runs any > slower/faster, etc. Ah, I'll check it out. In any case, I expect this to be way faster than the old method which used 3 times the memory (in-memory file, a copy on disk, and the decoded data in a growing vector.)
On 2017/02/10 at 17:54:00, rtoy wrote: > On 2017/02/10 17:48:34, DaleCurtis wrote: > > On 2017/02/09 at 23:32:46, rtoy wrote: > > > On 2017/02/09 23:24:43, DaleCurtis wrote: > > > > lgtm, nice work! Did you see how this performs on > > > > http://www.scirra.com/labs/bugs/webaudiodecode/ from > > http://crbug.com/424174 ? > > > > > > Wasn't that taken care of already when you switched Android to ffmpeg? > > > > Yes, but this approach doubles memory complexity so I was curious if it runs any > > slower/faster, etc. > > Ah, I'll check it out. In any case, I expect this to be way faster than the old method which used 3 times the memory (in-memory file, a copy on disk, and the decoded data in a growing vector.) I was curious relative to current shipping versions, not the old method. I.e. even on desktop did this change the perf characteristics?
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486744637902140,
"parent_rev": "698a09b008f29b551de793864a045f2ff0578857", "commit_rev":
"3489b2812bf4e670cc27627ee9a7a1fd1919f399"}
Message was sent while issue was closed.
Description was changed from ========== Decode entire in-memory file for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Read and decode the entire file, ignoring any estimated duration. This will result in increased memory consumption because the final output is built up in pieces before finally copying it to the final destination. Add tests for decoding webm files and WAV files using float32 samples. Vorbis tests need to be updated because the resulting outputs were previously truncated. BUG=673782 TEST=codec-tests/webm/webm-decode.html, codec-tests/wav/f32-44khz.html ========== to ========== Decode entire in-memory file for WebAudio DecodeAudioFileData assumed that any in-memory file to be decoded had a known duration that could be estimated without reading the entire file. For webm files produced by MediaRecorder, this can be false. Read and decode the entire file, ignoring any estimated duration. This will result in increased memory consumption because the final output is built up in pieces before finally copying it to the final destination. Add tests for decoding webm files and WAV files using float32 samples. Vorbis tests need to be updated because the resulting outputs were previously truncated. BUG=673782 TEST=codec-tests/webm/webm-decode.html, codec-tests/wav/f32-44khz.html Review-Url: https://codereview.chromium.org/2655783004 Cr-Commit-Position: refs/heads/master@{#449662} Committed: https://chromium.googlesource.com/chromium/src/+/3489b2812bf4e670cc27627ee9a7... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3489b2812bf4e670cc27627ee9a7...
Message was sent while issue was closed.
On 2017/02/09 23:24:43, DaleCurtis wrote: > lgtm, nice work! Did you see how this performs on > http://www.scirra.com/labs/bugs/webaudiodecode/ from http://crbug.com/424174 ? Ran these tests on a Nexus 9 (Android 6.0) Chrome 55: Ogg: 3.02 (avg of 3) AAC: 2.46 ToT: Ogg: 3.12 AAC: 2.79 While the averages look slower, I don't think they're statistically significant. The results from M55 seem to have a lot of variability (min = 2.18, max = 3.29 for ogg). So probably no change or at most 5-10% slower. |
