Bug 216584 – OfflineAudioContext constructor should not throw when given a bad buffer length

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

OfflineAudioContext constructor should not throw when given a bad buffer length. We should instead throw later on when trying to start rendering.

[Chris Dumez]

Created attachment 408864 [details]
Patch

[Chris Dumez]

Created attachment 408875 [details]
Patch

[Chris Dumez]

The GTK/WPE error does not make much sense to me. Maybe it needs a clean build.

[youenn fablet]

Comment on attachment 408875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408875&action=review

> Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:44
> +OfflineAudioDestinationNode::OfflineAudioDestinationNode(BaseAudioContext& context, unsigned numberOfChannels, AudioBuffer* renderTarget)

could be a RefPtr<AudioBuffer>&&?

> Source/WebCore/Modules/webaudio/WebKitOfflineAudioContext.cpp:39
> +inline WebKitOfflineAudioContext::WebKitOfflineAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget)

inline not needed

[youenn fablet]

retrying gtk build in case it works.

[Adrian Perez]

(In reply to Chris Dumez from comment #3)
> The GTK/WPE error does not make much sense to me. Maybe it needs a clean
> build.

It was a build issue masked by unified builds; after updating the patch
from bug #216599 it was possible for me to add this patch on top and have
builds complete for both the GTK and WPE ports.

[Carlos Garcia Campos]

Comment on attachment 408875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408875&action=review

> Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:88
> +WebKitAudioContext::WebKitAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget)
> +    : BaseAudioContext(document, renderTarget->numberOfChannels(), WTFMove(renderTarget))

I think this could be problematic, is it defined that the move happens after the ->numberOfChannels(). This has caused issues in the past with GCC. I think it would be safer if this receives the number of channels as a parameter too.

[Chris Dumez]

(In reply to Carlos Garcia Campos from comment #7)
> Comment on attachment 408875 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408875&action=review
> 
> > Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:88
> > +WebKitAudioContext::WebKitAudioContext(Document& document, Ref<AudioBuffer>&& renderTarget)
> > +    : BaseAudioContext(document, renderTarget->numberOfChannels(), WTFMove(renderTarget))
> 
> I think this could be problematic, is it defined that the move happens after
> the ->numberOfChannels(). This has caused issues in the past with GCC. I
> think it would be safer if this receives the number of channels as a
> parameter too.

A WTFMove() is just a cast though so it should not matter, even if the order is not guaranteed. The parameter type is an rvalue reference so the move constructor will not run until the constructor actually saves it into its data member.

[Chris Dumez]

Created attachment 408923 [details]
Patch

[Chris Dumez]

(In reply to Adrian Perez from comment #6)
> (In reply to Chris Dumez from comment #3)
> > The GTK/WPE error does not make much sense to me. Maybe it needs a clean
> > build.
> 
> It was a build issue masked by unified builds; after updating the patch
> from bug #216599 it was possible for me to add this patch on top and have
> builds complete for both the GTK and WPE ports.

It does not look like it did the trick. GTK/WPE bots still seem unhappy.

[Chris Dumez]

Created attachment 408924 [details]
Patch

[Chris Dumez]

Comment on attachment 408924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408924&action=review

> Source/WebCore/Modules/webaudio/WebKitOfflineAudioContext.h:35
> +    ~WebKitOfflineAudioContext();

Looks like adding this destructor fixed the WPE build.

[Chris Dumez]

Comment on attachment 408924 [details]
Patch

Clearing flags on attachment: 408924

Committed r267147: <https://trac.webkit.org/changeset/267147>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/68992445>