Bug 142676 – Add tolerance to WebAudio tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

WebAudio effects are intrinsically imprecise, so requiring bit for bit precision is not right.

rdar://problem/19996807
rdar://problem/15393179

[Alexey Proskuryakov]

Created attachment 248598 [details]
proposed fix

[WebKit Commit Bot]

Attachment 248598 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:168:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/Scripts/webkitpy/common/wavediff.py:34:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/common/wavediff.py:44:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/Scripts/webkitpy/common/wavediff.py:49:  multiple statements on one line (semicolon)  [pep8/E702] [5]
ERROR: Tools/Scripts/webkitpy/common/wavediff.py:107:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Created attachment 248612 [details]
with style fixes

[Alexey Proskuryakov]

Comment on attachment 248612 [details]
with style fixes

I need to update webkitpy tests.

[Tim Horton]

Comment on attachment 248612 [details]
with style fixes

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

> Tools/Scripts/webkitpy/common/wavediff.py:3
> +# Redistribution and use in source and binary forms, with or without

Is this the right license?

> Tools/Scripts/webkitpy/common/wavediff.py:43
> +            tempFile = tempfile.TemporaryFile()

what's the point of the tempFile local?

> Tools/Scripts/webkitpy/common/wavediff.py:48
> +            tempFile = tempfile.TemporaryFile()

should we use something like StringIO instead of writing the files to disk?

> Tools/Scripts/webkitpy/common/wavediff.py:64
> +        params1 = waveFile1.getparams()
> +        params2 = waveFile2.getparams()
> +
> +        channelCount1 = params1[0]
> +        frameCount1 = params1[3]
> +        sampleWidth1 = params1[1]
> +        channelCount2 = params2[0]
> +        frameCount2 = params2[3]
> +        sampleWidth2 = params2[1]

Is there a reason we do this? It's pretty ugly. Is using the getters a significant performance hit or something that inspired the uglier code? Like so:

channelCount1 = waveFile1.getnchannels()
frameCount1 = waveFile1.getframecount()
sampleWidth1 = waveFile1.getsampwidth()
channelCount2 = waveFile2.getnchannels()
frameCount2 = waveFile2.getframecount()
sampleWidth2 = waveFile2.getsampwidth()

I guess maybe it's because we want to use the params arrays again below? Still, random indexes make me :(

> Tools/Scripts/webkitpy/common/wavediff.py:79
> +            self._filesAreIdenticalWithinTolerance = not len(filter(lambda x: x > self._tolerance, results))

Not sure if we like comprehension syntax more or less than filter/lambda syntax...? (but I know which I like to read :D)

= not len([x for x in results if x > self._tolerance])

Also technically 'canonical' Python style prefers falsiness of the empty list to be the preferred way to determine emptiness instead of len(), but I kind of agree with Jer/whoever wrote this that that's not great.

[Daniel Bates]

Comment on attachment 248612 [details]
with style fixes

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

> Tools/Scripts/webkitpy/common/wavediff.py:71
> +        if self._filesAreIdentical:

For your consideration, I suggest we use an early return style such that we return early from this function if files are not identical (i.e. "not self._filesAreIdentical" evaluates to true). Among the benefits of this approach we can remove one level of indentation.

> Tools/Scripts/webkitpy/common/wavediff.py:77
> +            nDifferingSamples = len(filter(bool, results))

Can we come up with a better name for this variable? Maybe numberOfDifferingSamples?

> Tools/Scripts/webkitpy/common/wavediff.py:86
> +                self._diff += '\n'
> +                self._diff += 'Total differing samples: %d\n' % nDifferingSamples
> +                self._diff += 'Percentage of differing samples: %0.3f%%\n' % (100 * float(nDifferingSamples) / max(frameCount1, frameCount2))
> +                self._diff += 'Cumulative sample difference: %d\n' % cumulativeSampleDiff
> +                self._diff += 'Average sample difference: %f\n' % (float(cumulativeSampleDiff) / nDifferingSamples)

Maybe using a single multiline string and str.format() instead of concatenating multiple strings with string substitution would improve the readability of this code?

self._diff = """
Total differing samples: {numberOfDifferingSamples}
Percentage of differing samples: {percentOfDifferingSamples:0.3f}%
Cumulative sample difference: {cumulativeSampleDifference}
Average sample difference: {averageSampleDifference}
""".format(numberOfDifferingSamples=nDifferingSamples,
           percentOfDifferingSamples=100 * float(nDifferingSamples) / max(frameCount1, frameCount2),
           cumulativeSampleDifference=cumulativeSampleDiff,
           averageSampleDifference=float(cumulativeSampleDiff) / nDifferingSamples)

> Tools/Scripts/webkitpy/common/wavediff.py:97
> +    def _readSamples(self, file, sampleWidth, nSamples):

This method does not make use of any instance variables. You may want to consider making this a static method by adding the @staticmethod decorator (above this function) and removing the first parameter. See <https://docs.python.org/2/library/functions.html#staticmethod> for more details.

> Tools/Scripts/webkitpy/common/wavediff.py:108
> +            self._filesAreIdentical = False

Is this correct? I don't see the value of setting this instance variable given that this function is considered private (given the presence of the prefix of '_' at the start of its name), it is only called on line 74, and we override self._filesAreIdentical on line 78.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:166
> +        diff = WaveDiff(expected_audio, actual_audio)

I suggest we inline the value of this variable on line 168 since it is only referenced on that line and I do not feel that the name of this variable makes the code more readable given that the name of the class, WaveDiff, is reasonably descriptive.

> Tools/Scripts/webkitpy/port/base.py:289
> +        diff = WaveDiff(expected_audio, actual_audio)
> +        return not diff.filesAreIdenticalWithinTolerance()

Ditto.

[Alexey Proskuryakov]

Created attachment 248735 [details]
with fixes

Made all the suggested improvements, except for two:

> Not sure if we like comprehension syntax more or less than filter/lambda syntax...? (but I know which I like to read :D)

Kept as is for consistency, because there is another instance of filter just below ("len(filter(bool, results))").

> Maybe using a single multiline string and str.format() instead of concatenating multiple strings with string substitution would improve the readability of this code?

Unsure, the proposed variant seemed less readable to me. I can make the change if you feel strongly about it.

[Tim Horton]

Comment on attachment 248735 [details]
with fixes

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

> Tools/Scripts/webkitpy/common/wavediff.py:94
> +        unpackFormat = 'b' if sampleWidth == 1 else 'h'

maybe we should assert here that it's 1 or 2?

> Tools/Scripts/webkitpy/port/test.py:101
> -TOTAL_TESTS = 71
> +TOTAL_TESTS = 72

:(

[Alexey Proskuryakov]

Comment on attachment 248735 [details]
with fixes

I don't think that .wav supports anything but 8-bit and 16-bit.

[WebKit Commit Bot]

Comment on attachment 248735 [details]
with fixes

Clearing flags on attachment: 248735

Committed r181579: <http://trac.webkit.org/changeset/181579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.