First sound test #11887

daschuer · 2023-08-29T13:15:28Z

In an attempt to fix #11094 I have created a test that compares the first sound position of the current version with the position of future versions.

The idea was to replace the faulty 10.0.20348.1 Media Foundation version with FFmpeg.
This test exposes that this is not trivial, because the Media Foundation has an offset compared to FFmpeg according to a not yet known pattern.

We need further investigations to either fix #11094 with Media Foundation or mimic the offset behaviour with FFmpeg. The alternative would be to just swap to FFmpeg and tell the users to reanalyse the track and adust the cue points again.

In addition this has discovered a regression in the 2.3 branch where the FPM_64BIT has been lost:
https://github.com/mixxxdj/buildserver/blob/2.3.x-windows/build/libmad-0.15.1b/msvc%2B%2B/libmad.vcxproj#L133
This falls back to 32 bit maths leading to significant rounding issues.
https://github.com/markjeee/libmad/blob/c2f96fa4166446ac99449bdf6905f4218fb7d6b5/fixed.h#L425
This version is the most portable but it loses significant accuracy.

This PR is just a test that aims to detect offset issues due to decoder update early and should be merged independently.

daschuer · 2023-08-30T13:19:25Z

The new clang format insist to break the format. I suggest to just merge it as it is. I have no interest to debug it, since we run clang format on changes only.

…fter updating soundsources.

daschuer · 2023-08-31T06:26:41Z

Rebasing to the updated updated pre-commit does not help. I give up.

Swiftb0y · 2023-08-31T12:28:21Z

Wrap the ifdef'd block with // clang-format off/on then if you insist on your formatting.

daschuer · 2023-08-31T21:33:11Z

I have no interest to maintain clang-format exceptions because they break nice formatted comments as well.
I just went ahead and applied the clang-format suggestions.

daschuer · 2023-09-03T22:31:19Z

This is now adjusted for the FPM_64BIT libmad version and merged from 2.4.
Ready for review and merge.

Swiftb0y · 2023-09-01T13:16:00Z

CMakeLists.txt

@@ -2797,6 +2797,7 @@ if(COREAUDIO)
    src/sources/soundsourcecoreaudio.cpp
    src/sources/v1/legacyaudiosourceadapter.cpp
    lib/apple/CAStreamBasicDescription.cpp
+    src/util/macosversion.mm


I don't think this should be dependent on coreaudio... it'll probably get used somewhere else too in the future.

Swiftb0y · 2023-09-01T13:19:22Z

src/sources/soundsourcemp3.cpp

+                    } else if (m_madStream.error == MAD_ERROR_BADDATAPTR &&
+                            m_curFrameIndex == firstFrameIndex) {
+                        // This is expected after starting decoding with an offset


expected by us, but is it by MAD? Are we using MAD wrong here?

I have debugged into that and there is is nothing wrong. That why I have silenced the message.
You find this in other adoptions as well. Here for instance:
https://github.com/Sneeds-Feed-and-Seed/sneedacity/blob/5fba67545ca29d009b51d355687979a3996d252c/src/import/ImportMP3.cpp#L1093

src/sources/soundsourcemediafoundation.cpp

Swiftb0y · 2023-09-04T12:28:43Z

src/sources/soundsourcemediafoundation.cpp

+        UINT size;
+        DWORD* fi;
+        if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size) {


either check all, or none;

Suggested change

UINT size;

DWORD* fi;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size) {

UINT size = 0;

DWORD* fi = nullptr;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size > 0 && fi != nullptr) {

Suggested change

UINT size;

DWORD* fi;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size) {

UINT size;

DWORD* fi;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size)) {

Swiftb0y · 2023-09-04T12:35:30Z

src/sources/soundsourcemediafoundation.cpp

+        DWORD* fi;
+        if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size) {
+            const VS_FIXEDFILEINFO* verInfo = reinterpret_cast<const VS_FIXEDFILEINFO*>(fi);


Suggested change

DWORD* fi;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&fi), &size) && size) {

const VS_FIXEDFILEINFO* verInfo = reinterpret_cast<const VS_FIXEDFILEINFO*>(fi);

// iff VerQueryValue is called with "\", the version info is of type VS_FIXEDFILEINFO

VS_FIXEDFILEINFO* verInfo;

if (VerQueryValue(info.data(), L"\\", reinterpret_cast<void**>(&verInfo) && size) && size) {

Swiftb0y · 2023-09-04T12:37:37Z

src/sources/soundsourcemediafoundation.cpp

+            return QStringLiteral("%1.%2.%3.%4")
+                    .arg(HIWORD(verInfo->dwProductVersionMS))
+                    .arg(LOWORD(verInfo->dwProductVersionMS))
+                    .arg(HIWORD(verInfo->dwProductVersionLS))
+                    .arg(LOWORD(verInfo->dwProductVersionLS));


avoid unnecessary chaining

Suggested change

return QStringLiteral("%1.%2.%3.%4")

.arg(HIWORD(verInfo->dwProductVersionMS))

.arg(LOWORD(verInfo->dwProductVersionMS))

.arg(HIWORD(verInfo->dwProductVersionLS))

.arg(LOWORD(verInfo->dwProductVersionLS));

return QStringLiteral("%1.%2.%3.%4")

.arg(HIWORD(verInfo->dwProductVersionMS),

LOWORD(verInfo->dwProductVersionMS),

HIWORD(verInfo->dwProductVersionLS),

LOWORD(verInfo->dwProductVersionLS));

Swiftb0y · 2023-09-04T12:47:38Z

src/test/soundproxy_test.cpp

+            {QStringLiteral("cover-test.wav"), 1166},
+            {QStringLiteral("cover-test.wv"), 1166}};
+
+    for (const auto& ref : refs) {


have you considered writing a parametrized test? That would have the advantage that the test would not abort after the first failure and each first sound ref would be tested independently.

https://github.com/google/googletest/blob/main/docs/advanced.md#how-to-write-value-parameterized-tests

The test already continues when one test is failing. I am fine with the current implementation.

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Swiftb0y · 2023-09-04T14:19:28Z

src/sources/soundsourcemediafoundation.cpp

+            return QStringLiteral("%1.%2.%3.%4").arg(
+                    QString::number(HIWORD(pVerInfo->dwProductVersionMS)),
+                    QString::number(LOWORD(pVerInfo->dwProductVersionMS)),
+                    QString::number(HIWORD(pVerInfo->dwProductVersionLS)),
+                    QString::number(LOWORD(pVerInfo->dwProductVersionLS)));


Really looking forward to Qt6 when we finally have a variadic QString::arg...

daschuer · 2023-09-04T21:55:15Z

The failing test confirms that the Media Foundation version is still read:
D:/a/mixxx/mixxx/src/test/id3-test-data/cover-test-ffmpeg-aac.m4a Microsoft Media Foundation 10.0.17763.2989
https://github.com/mixxxdj/mixxx/actions/runs/6077689437/job/16487777138?pr=11887

daschuer · 2023-09-04T23:25:09Z

All green now.

Swiftb0y

any1 else wanting to take a look?

JoergAtGithub

LGTM!

JoergAtGithub · 2023-09-05T18:31:30Z

Thank you!

daschuer added this to the 2.4.0 milestone Aug 29, 2023

github-actions bot added build code quality labels Aug 29, 2023

This was referenced Aug 29, 2023

Mp3 rounding issues, low accuracy #11888

Closed

Some SoundSourceProxyTests fail on Win11 with VS2022 #11094

Open

daschuer added 9 commits August 31, 2023 08:11

Report failing index in expectDecodedSamplesEqual()

83e9ae0

clean up getOrInitTestDir()

435691a

Print FFmpeg version

6447a1a

Print Apple CoreAudio Version

142f84a

Print Media Foundation version

b851dd6

Print MAD version

58ff6c5

Improve log output with MAD

0df7471

Add SoundSourceProxyTest firstSoundTest() that aims to find offsets a…

ffaf2db

…fter updating soundsources.

Document result from different platforms

6d71453

daschuer force-pushed the firstSoundTest branch from b5b216a to 37a86ed Compare August 31, 2023 06:24

daschuer added 2 commits August 31, 2023 22:58

Use the current ofsets as reference

148269d

Apply broken indent that pleases clang-format

ff0d20e

daschuer force-pushed the firstSoundTest branch from 935e4f7 to ff0d20e Compare August 31, 2023 21:00

daschuer mentioned this pull request Aug 31, 2023

[libmad] Detect the FPM flag according to the CMAKE_SYSTEM_PROCESSOR microsoft/vcpkg#33490

Merged

7 tasks

daschuer mentioned this pull request Sep 3, 2023

Update to build environment with fixed libmad on windows #11911

Merged

daschuer added 2 commits September 3, 2023 09:58

Adjust the windows offset for MAD with FPM_64BIT

c1145f1

Merge remote-tracking branch 'upstream/2.4' into firstSoundTest

1780add

Swiftb0y reviewed Sep 4, 2023

View reviewed changes

daschuer and others added 2 commits September 4, 2023 15:16

Move macosversion.mm to the APPLE region

2a59ada

Warn in case of GetFileVersionInfoSize() error

2bc18ad

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

Swiftb0y reviewed Sep 4, 2023

View reviewed changes

daschuer added 2 commits September 4, 2023 23:17

improve VerQueryValue() call

5b545c1

Don't chain arg()

8526157

daschuer force-pushed the firstSoundTest branch from 4659439 to f687745 Compare September 4, 2023 21:19

Merge remote-tracking branch 'upstream/2.4' into firstSoundTest

96b00a7

daschuer force-pushed the firstSoundTest branch from f687745 to 96b00a7 Compare September 4, 2023 22:09

Swiftb0y approved these changes Sep 4, 2023

View reviewed changes

JoergAtGithub approved these changes Sep 5, 2023

View reviewed changes

JoergAtGithub merged commit a428aae into mixxxdj:2.4 Sep 5, 2023

daschuer deleted the firstSoundTest branch September 6, 2023 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First sound test #11887

First sound test #11887

daschuer commented Aug 29, 2023

daschuer commented Aug 30, 2023

daschuer commented Aug 31, 2023

Swiftb0y commented Aug 31, 2023

daschuer commented Aug 31, 2023 •

edited

Loading

daschuer commented Sep 3, 2023

Swiftb0y Sep 1, 2023

Swiftb0y Sep 1, 2023

daschuer Sep 4, 2023

Swiftb0y Sep 4, 2023

Swiftb0y Sep 4, 2023

Swiftb0y Sep 4, 2023

Swiftb0y Sep 4, 2023

daschuer Sep 4, 2023

Swiftb0y Sep 4, 2023

daschuer commented Sep 4, 2023

daschuer commented Sep 4, 2023

Swiftb0y left a comment

JoergAtGithub left a comment

JoergAtGithub commented Sep 5, 2023

First sound test #11887

First sound test #11887

Conversation

daschuer commented Aug 29, 2023

daschuer commented Aug 30, 2023

daschuer commented Aug 31, 2023

Swiftb0y commented Aug 31, 2023

daschuer commented Aug 31, 2023 • edited Loading

daschuer commented Sep 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Sep 4, 2023

daschuer commented Sep 4, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

JoergAtGithub left a comment

Choose a reason for hiding this comment

JoergAtGithub commented Sep 5, 2023

daschuer commented Aug 31, 2023 •

edited

Loading