Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1464673002:
Support multichannel audio stream more than 2 in MediaStreamDestinationNode (Closed)
|
Created:
5 years, 1 month ago by hongchan Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, blink-reviews, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport multichannel audio stream more than 2 in MediaStreamDestinationNode
WebAudioCapturerSource does not have the support for multichannel audio bus
greater than 2. This CL is to implement the support up to 8 channels in
WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode
before passing the multichannel audio bus.
I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220
BUG=557185
TEST=
LayoutTests/webaudio/mediastreamaudiodestinationnode.html
ManualTests/webaudio/multichannel-mediastreamdestination.html
Committed: https://crrev.com/5b2c345c7e535ca7e4024eb09fedda3b30b9b924
Cr-Commit-Position: refs/heads/master@{#362215}
Patch Set 1 #Patch Set 2 : Added tests #
Total comments: 20
Patch Set 3 : Addressed feedback from rtoy #
Total comments: 3
Patch Set 4 : Fixed intdentation #
Messages
Total messages: 24 (10 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. NOTE: A manual layout test is expected. BUG=557185 TEST=TBD ========== to ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ==========
hongchan@chromium.org changed reviewers: + dalecurtis@chromium.org, rtoy@chromium.org
PTAL. I am still figuring out the right way to test this. Please feel free to throw your idea on the testing.
https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... File third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:26: <p>This test is for multichannel (> 2 channels) support for Do you need to change ">" to ">"? https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:65: maxChannelCount < 8 ? maxChannelCount : 8; I think this code would be more robust if you added a try/catch block to catch the error if setting the channel count fails. Then this code will do the right thing if we change the max count of MediaStreamDestination in the future. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:122: // collected and the |onended| event won't be fired. File a bug on this, and put a link to the bug here. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:76: Why a new blank line here? https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:103: // Update the pipeline wit the new channel count only if absolutely Typo: "wit" -> "with" https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:107: // have access to m_mixBus. I think you should lock the context here. Or maybe a process lock should be added so that you don't change the m_mixBus out from under the process() function above. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:114: unsigned long MediaStreamAudioDestinationHandler::maxChannelCount() const Is this used anywhere else? If not, is there a reason not to use kMaxChannelCount directly? https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h:62: // This internal mix bus functions as a up/down mixing buffer. This isn't quite descriptive enough. I think it should say it for mixing the input to the actual number of channels in the destination.
https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... File third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:26: <p>This test is for multichannel (> 2 channels) support for On 2015/11/20 14:47:18, Raymond Toy wrote: > Do you need to change ">" to ">"? Although the DOM parser is smart enough to do this for me, DONE. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:65: maxChannelCount < 8 ? maxChannelCount : 8; On 2015/11/20 14:47:18, Raymond Toy wrote: > I think this code would be more robust if you added a try/catch block to catch > the error if setting the channel count fails. Then this code will do the right > thing if we change the max count of MediaStreamDestination in the future. Done. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:122: // collected and the |onended| event won't be fired. On 2015/11/20 14:47:18, Raymond Toy wrote: > File a bug on this, and put a link to the bug here. Done. https://code.google.com/p/chromium/issues/detail?id=559220 However, it seems to be still flaky even after I added the global storage for oscillators. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:76: On 2015/11/20 14:47:18, Raymond Toy wrote: > Why a new blank line here? Because the comment above only applies to the line 75. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:103: // Update the pipeline wit the new channel count only if absolutely On 2015/11/20 14:47:18, Raymond Toy wrote: > Typo: "wit" -> "with" Done. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:107: // have access to m_mixBus. On 2015/11/20 14:47:18, Raymond Toy wrote: > I think you should lock the context here. Or maybe a process lock should be > added so that you don't change the m_mixBus out from under the process() > function above. Yes I agree. I will add the lock here. However, note that DefaultAudioDestinatioNode::setChannelCount also does not have a lock in the same scenario. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Perhaps it is safe because of stopping/recreating the destination? https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:114: unsigned long MediaStreamAudioDestinationHandler::maxChannelCount() const On 2015/11/20 14:47:18, Raymond Toy wrote: > Is this used anywhere else? If not, is there a reason not to use > kMaxChannelCount directly? I don't have a strong opinion on this and I am simply following conventions. You prefer to have kMaxChannelCount rather than a getter? https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h:62: // This internal mix bus functions as a up/down mixing buffer. On 2015/11/20 14:47:19, Raymond Toy wrote: > This isn't quite descriptive enough. I think it should say it for mixing the > input to the actual number of channels in the destination. Done.
media/ lgtm https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... File content/renderer/media/webaudio_capturer_source.cc (right): https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... content/renderer/media/webaudio_capturer_source.cc:43: media::CHANNEL_LAYOUT_DISCRETE : fix indent.
https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... File content/renderer/media/webaudio_capturer_source.cc (right): https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... content/renderer/media/webaudio_capturer_source.cc:43: media::CHANNEL_LAYOUT_DISCRETE : On 2015/11/24 21:48:11, DaleCurtis wrote: > fix indent. Done. However, the indentation is not consistent in this file. What are the rules in the content renderer?
lgtm with nit. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... File third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:122: // collected and the |onended| event won't be fired. On 2015/11/20 18:02:59, hoch wrote: > On 2015/11/20 14:47:18, Raymond Toy wrote: > > File a bug on this, and put a link to the bug here. > > Done. > > https://code.google.com/p/chromium/issues/detail?id=559220 > > However, it seems to be still flaky even after I added the global storage for > oscillators. Add link to this bug in the comments. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:76: On 2015/11/20 18:02:59, hoch wrote: > On 2015/11/20 14:47:18, Raymond Toy wrote: > > Why a new blank line here? > > Because the comment above only applies to the line 75. Acknowledged. https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:114: unsigned long MediaStreamAudioDestinationHandler::maxChannelCount() const On 2015/11/20 18:02:59, hoch wrote: > On 2015/11/20 14:47:18, Raymond Toy wrote: > > Is this used anywhere else? If not, is there a reason not to use > > kMaxChannelCount directly? > > I don't have a strong opinion on this and I am simply following conventions. You > prefer to have kMaxChannelCount rather than a getter? Up to you. This isn't speed-critical, so I'm fine with it. Getters are probably better in general.
Description was changed from ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ========== to ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220 BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ==========
https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... File third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html (right): https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:122: // collected and the |onended| event won't be fired. On 2015/11/30 18:06:09, Raymond Toy (OOO to Nov 30) wrote: > On 2015/11/20 18:02:59, hoch wrote: > > On 2015/11/20 14:47:18, Raymond Toy wrote: > > > File a bug on this, and put a link to the bug here. > > > > Done. > > > > https://code.google.com/p/chromium/issues/detail?id=559220 > > > > However, it seems to be still flaky even after I added the global storage for > > oscillators. > > Add link to this bug in the comments. This bug is not directly related to this CL, so I will mention it in the description.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464673002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464673002/80001
https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... File content/renderer/media/webaudio_capturer_source.cc (right): https://codereview.chromium.org/1464673002/diff/60001/content/renderer/media/... content/renderer/media/webaudio_capturer_source.cc:43: media::CHANNEL_LAYOUT_DISCRETE : On 2015/11/30 18:04:50, hoch wrote: > On 2015/11/24 21:48:11, DaleCurtis wrote: > > fix indent. > > Done. However, the indentation is not consistent in this file. What are the > rules in the content renderer? Whatever git cl format says :)
On 2015/11/30 18:10:44, hoch wrote: > https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... > File > third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html > (right): > > https://codereview.chromium.org/1464673002/diff/40001/third_party/WebKit/Manu... > third_party/WebKit/ManualTests/webaudio/multichannel-mediastreamdestination.html:122: > // collected and the |onended| event won't be fired. > On 2015/11/30 18:06:09, Raymond Toy (OOO to Nov 30) wrote: > > On 2015/11/20 18:02:59, hoch wrote: > > > On 2015/11/20 14:47:18, Raymond Toy wrote: > > > > File a bug on this, and put a link to the bug here. > > > > > > Done. > > > > > > https://code.google.com/p/chromium/issues/detail?id=559220 > > > > > > However, it seems to be still flaky even after I added the global storage > for > > > oscillators. > > > > Add link to this bug in the comments. > > This bug is not directly related to this CL, so I will mention it in the > description. I wouldn't have, because issue has nothing to do with what you're changing here. But the bug is relevant to the test because you re-arranged it from what you had originally because of it.
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/1464673002/#ps100001 (title: "Fixed intdentation")
Dry run: Commit queue failed due to new patchset.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464673002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464673002/100001
Message was sent while issue was closed.
Description was changed from ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220 BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ========== to ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220 BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220 BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html ========== to ========== Support multichannel audio stream more than 2 in MediaStreamDestinationNode WebAudioCapturerSource does not have the support for multichannel audio bus greater than 2. This CL is to implement the support up to 8 channels in WebAudioCapturerSource, also put checks/blocks in MediaStreamDestinationNode before passing the multichannel audio bus. I also discovered the flakiness of onended event in OscillatorNode while I was creating a manual test for this CL: https://code.google.com/p/chromium/issues/detail?id=559220 BUG=557185 TEST= LayoutTests/webaudio/mediastreamaudiodestinationnode.html ManualTests/webaudio/multichannel-mediastreamdestination.html Committed: https://crrev.com/5b2c345c7e535ca7e4024eb09fedda3b30b9b924 Cr-Commit-Position: refs/heads/master@{#362215} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5b2c345c7e535ca7e4024eb09fedda3b30b9b924 Cr-Commit-Position: refs/heads/master@{#362215} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
