| Summary: | Layout Test webaudio/panner-set-model.html crashes on debug Chromium bots | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||
| Component: | Tools / Tests | Assignee: | Raymond Toy <rtoy> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | crogers, dglazkov, rtoy, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 77235 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Levi Weintraub
2012-01-28 13:24:21 PST
This is caused at line 57 in platform/audio/Panner.cpp. The sound field panning model is not implemented, which the new test actually tests. Chris, what is the correct approach here? Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model? (In reply to comment #1) > This is caused at line 57 in platform/audio/Panner.cpp. The sound field panning model is not implemented, which the new test actually tests. > > Chris, what is the correct approach here? Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model? The correct approach is to not call ASSERT_NOT_REACHED, which should only be used to assert bugs and logic errors in the C++ code. Ideally, we would be throwing an exception here, but another option is to log a console message similar to what we do in AudioBufferSourceNode when the deprecated "looping" attribute is used. Even better yet, we'll implement the SOUNDFIELD algorithm! (In reply to comment #2) > (In reply to comment #1) > > This is caused at line 57 in platform/audio/Panner.cpp. The sound field panning model is not implemented, which the new test actually tests. > > > > Chris, what is the correct approach here? Don't try to test if we can set the model to sound field, or modify Panner.cpp so that we silently return nullptr (without the ASSERT_NOT_REACHED) for the sound field model? > > The correct approach is to not call ASSERT_NOT_REACHED, which should only be used to assert bugs and logic errors in the C++ code. Ideally, we would be throwing an exception here, but another option is to log a console message similar to what we do in AudioBufferSourceNode when the deprecated "looping" attribute is used. Ok. I'll add a log message for this. The exception part can be handled when https://bugs.webkit.org/show_bug.cgi?id=77235 is addressed. (Will add a comment there to remind us about the sound field model.) > > Even better yet, we'll implement the SOUNDFIELD algorithm! Sounds like another bug report to be created. :-) Created attachment 124620 [details]
Patch
Comment on attachment 124620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review > Source/WebCore/platform/audio/Panner.cpp:57 > + return nullptr; We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug# In other words, we can't just allow m_panner to be set to nullptr and get crashes later on! (In reply to comment #5) > (From update of attachment 124620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review > > > Source/WebCore/platform/audio/Panner.cpp:57 > > + return nullptr; > > We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug# Ok. > > In other words, we can't just allow m_panner to be set to nullptr and get crashes later on! Actually, it doesn't crash. I tested this with the panner/reverb demo and manually set the panning model to 2 via the javascript console. Demo continues, just without audio. (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 124620 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=124620&action=review > > > > > Source/WebCore/platform/audio/Panner.cpp:57 > > > + return nullptr; > > > > We can't just return nullptr here -- the check for PanningModelSoundField needs to happen in AudioPannerNode::setPanningModel() in the corresponding "case" in the switch statement for SOUNDFIELD -- where we need to silently fail (and include the appropriate comment with bug# > > Ok. > > > > In other words, we can't just allow m_panner to be set to nullptr and get crashes later on! > > Actually, it doesn't crash. I tested this with the panner/reverb demo and manually set the panning model to 2 via the javascript console. Demo continues, just without audio. But it should fail without side-effects. It should just completely *ignore* SOUNDFIELD and not disturb the panning from the previous algorithm Created attachment 124805 [details]
Patch
Updated according to review. AudioPannerNode now does nothing if SOUNDFIELD is selected, except that a warning message is sent to the Javascript console. Debug builds don't crash anymore. Comment on attachment 124805 [details] Patch Attachment 124805 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11389123 New failing tests: webaudio/panner-set-model.html (In reply to comment #10) > (From update of attachment 124805 [details]) > Attachment 124805 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11389123 > > New failing tests: > webaudio/panner-set-model.html I don't understand this failure. The output doesn't show anything about panner-set-model. Created attachment 124968 [details]
Update expected results to match actual which now includes a console message
As discussed with Chris, we would rather fix this issue in bug 77235 and throw an exception instead of silently doing nothing. When the patch for 77235 has landed, we should be able to close out this bug. Bug 77235 has landed. Closing this. Comment on attachment 124968 [details] Update expected results to match actual which now includes a console message Cleared review? from attachment 124968 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). (In reply to comment #15) > (From update of attachment 124968 [details]) > Cleared review? from attachment 124968 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). Sorry. I closed the bug when bug 77235 landed but forget to remove the review flags. |