WebAudio effects are intrinsically imprecise, so requiring bit for bit precision is not right. rdar://problem/19996807 rdar://problem/15393179
Created attachment 248598 [details] proposed fix
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.
Created attachment 248612 [details] with style fixes
Comment on attachment 248612 [details] with style fixes I need to update webkitpy tests.
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.
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.
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.
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 :(
Comment on attachment 248735 [details] with fixes I don't think that .wav supports anything but 8-bit and 16-bit.
Comment on attachment 248735 [details] with fixes Clearing flags on attachment: 248735 Committed r181579: <http://trac.webkit.org/changeset/181579>
All reviewed patches have been landed. Closing bug.