1270055 - Unaligned buffer used in DynamicsCompressor

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Dan Minor [:dminor]]

This looks like what is causing this crash:

https://crash-stats.mozilla.com/report/index/fe2f58ec-0743-497e-9cd8-1ba2b2160503

Comment 1

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1270055 - Unaligned buffer used in DynamicsCompressor; r?padenot

Review commit: https://reviewboard.mozilla.org/r/50413/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50413/

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8748607 [details]
MozReview Request: Bug 1270055 - Unaligned buffer used in DynamicsCompressor; r?padenot

https://reviewboard.mozilla.org/r/50413/#review47225

Comment 3

[Karl Tomlinson (:karlt)]

I guess we are missing tests for DynamicsCompressor with a Gain on the input.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8eb1b8cdc1

Comment 5

[Dan Minor [:dminor]]

This is also a bug in 48 but the alignment checks in AudioNodeEngine.cpp present in 48 prevent it from causing problems there, so I don't think it is worth fixing.

Comment 6

[Dan Minor [:dminor]]

Attachment: Test case with gain

Comment 7

[Dan Minor [:dminor]]

Try run with the new test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea32238df41f1ed33d8b84066a9aea9b08f1bad7&selectedJob=20413509

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d8eb1b8cdc1

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8749171 [details] [diff] [review]
Test case with gain

Thanks, Dan!

I don't know what all these numbers mean.  Can you add a comment, please, to
explain where the min and max peak values come?

I wouldn't worry, now that you have written this, but it would be good to put
new tests under testing/web-platform where appropriate.  Even if the test is
more suitable for running only in Gecko, the testharness type (now the default
for gen_template.pl) is often better if no special powers are required,
because it can be easily ported to web-platform tests.  testharness tests in
dom/media/webaudio run with the mochitests.

I never understood why so many web audio mochitests use addLoadEvent().

Comment 10

[Dan Minor [:dminor]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #9)
> Comment on attachment 8749171 [details] [diff] [review]
> Test case with gain
> 
> Thanks, Dan!
> 
> I don't know what all these numbers mean.  Can you add a comment, please, to
> explain where the min and max peak values come?
> 

Will do.

> I wouldn't worry, now that you have written this, but it would be good to put
> new tests under testing/web-platform where appropriate.  Even if the test is
> more suitable for running only in Gecko, the testharness type (now the
> default
> for gen_template.pl) is often better if no special powers are required,
> because it can be easily ported to web-platform tests.  testharness tests in
> dom/media/webaudio run with the mochitests.
> 
> I never understood why so many web audio mochitests use addLoadEvent().

Thanks for the tip. I've been cargo-culting existing mochitests when making new tests, I'll look at switching over to testharness tests next time.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc4c1ab9484

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bdc4c1ab9484