New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix writing of track metadata on Windows #4586

Merged

uklotzde merged 11 commits into mixxxdj:2.3 from daschuer:lp1955331

Dec 30, 2021

Member

daschuer commented Dec 27, 2021 •

edited

Loading

This fixes https://bugs.launchpad.net/mixxx/+bug/1955314 and https://bugs.launchpad.net/mixxx/+bug/1955331.

This is a mayor bugfix that should be released soon. Because the track metadata was not exported on windows when the track has not been loaded into a deck. Since the user assumes that the metadata is written, there is a risk of data loss. This was discovered in the first attempt for a fix here: #4572

The fix makes use of the ReplaceFileW() API function that is recommended instead of the deprecated transition feature for atomic file operations. ReplaceFileW() avoids virus scanner and indexer locking and keeps the file creation time at its old value.

The refactoring part has been issued here #4584 for the main branch.

daschuer added 4 commits

December 23, 2021 14:50


          Use dedicated ReplaceFileW API function when relpacing the track afte…

231c0c2

…r writing

This aims to fix the lost file creation time and the file access issue


          Handle ERROR_SHARING_VIOLATION and ERROR_ACCESS_DENIED

0cbedfe


          Added a retry loop in case of a temprary ERROR_SHARING_VIOLATION

8e1fb69


          Close track file after reading stream data before metadata export

c910a3e

This fixes metadata export on Windows when the track not been played before.

daschuer requested a review from uklotzde

December 27, 2021 22:10

uklotzde suggested changes

View reviewed changes

src/sources/metadatasourcetaglib.cpp Outdated

+                                          << "by"
+                                          << m_tempFileName
+                                          << "because it is used by another process";
+                                  Sleep(kWindowsSharingViolationsSleepMs); // Note: Capital S for Windows API Sleep()

Contributor

uklotzde Dec 27, 2021

Why don't you use QThread::msleep() here?

src/sources/metadatasourcetaglib.cpp Outdated

+              // After Mixxx has closed the track file, the indexer or virus scanner
+              // might kick in and fail ReplaceFileW() with a sharing violation when
+              // replacing the original file with the one with the updated metadata.
+              const int kWindowsSharingViolationsRetries = 5;

Contributor

uklotzde Dec 27, 2021

kWindowsSharingViolationMaxRetries

src/sources/metadatasourcetaglib.cpp Outdated

+              // might kick in and fail ReplaceFileW() with a sharing violation when
+              // replacing the original file with the one with the updated metadata.
+              const int kWindowsSharingViolationsRetries = 5;
+              const int kWindowsSharingViolationsSleepMs = 100;

Contributor

uklotzde Dec 27, 2021

kWindowsSharingViolationSleepBeforeNextRetryMillis

src/sources/metadatasourcetaglib.cpp Outdated

+                                  return false;
+                              case ERROR_SHARING_VIOLATION:
+                                  // The process cannot access the file because it is being used by another process.
+                                  kLogger.critical()

Contributor

uklotzde Dec 27, 2021

Not critical if a retry is following.

src/sources/metadatasourcetaglib.cpp Outdated

+                                  return false;
+                              }
+                          }
+                          break;

Contributor

uklotzde Dec 27, 2021

It is difficult to recognize that this break belongs to the loop. Is it actually needed?

src/sources/metadatasourcetaglib.cpp Outdated

               // potential failures caused by exceeded path length.
               const QString kSafelyWritableOrigFileSuffix = QStringLiteral("_orig");
+              #ifdef __WINDOWS__
+              // After Mixxx has closed the track file, the indexer or virus scanner

Contributor

uklotzde Dec 27, 2021

This comment belongs to the code where these constants are actually used, but not to these constants. Mentioning why ReplaceFileW() is needed here but not in the context were it is actually invoked is confusing.

src/sources/soundsourceproxy.cpp Outdated

                               pTrack->updateStreamInfoFromSource(
                                       pAudioSource->getStreamInfo());
                               DEBUG_ASSERT(pTrack->hasStreamInfoFromSource());
+                              // Make sure that the track file is closes after reading

Contributor

uklotzde Dec 27, 2021

multiple typos

src/sources/soundsourceproxy.cpp Outdated

                               // before exporting metadata. In this case the caller (this class)
                               // is responsible for updating the stream info if needed.
                               if (!m_pTrack) {
+                                  // TODO() It is surprising to receive here a m_pSoundSource;

Contributor

uklotzde Dec 27, 2021

TODO is not a function
"receive"?
semicolon to end a comment?

uklotzde added the major bug label

daschuer added 3 commits

December 28, 2021 21:13


          Replace Sleep() by QThread::msleep()

3de1051


          Improve variable naming

d2f50b6


          Improve control flow and comments

66b178f

Member Author

daschuer commented Dec 28, 2021

Done

ronso0 added the changelog label

uklotzde reviewed

View reviewed changes

src/sources/soundsourceproxy.cpp Outdated

                               // before exporting metadata. In this case the caller (this class)
                               // is responsible for updating the stream info if needed.
                               if (!m_pTrack) {
+                                  // TODO: It is surprising that openAudioSource returns a m_pSoundSource

Contributor

uklotzde Dec 29, 2021

Why is it suprising?? SoundSource is an extension of both AudioSource and MetadataSource, basic OO design.

Contributor

uklotzde Dec 29, 2021

This should work and fix the bug with the open file:

m_pAudioSource = m_pSoundSource;
return m_pAudioSource;

Member Author

daschuer Dec 29, 2021

I have already fixed the underlying issue in #4584 please have a look.

uklotzde suggested changes

View reviewed changes

src/sources/soundsourceproxy.cpp

                               pTrack->updateStreamInfoFromSource(
                                       pAudioSource->getStreamInfo());
                               DEBUG_ASSERT(pTrack->hasStreamInfoFromSource());
+                              // Make sure that the track file is closed after reading.

Contributor

uklotzde Dec 29, 2021

This additional code should become obsolete with the fix I just proposed.

Contributor

uklotzde Dec 29, 2021

This is still needed:

proxy.closeAudioSource();

Member Author

daschuer Dec 29, 2021

I would like to keep this as an hot fix solution with a minimum impact.

The rest is fixed in #4584

Contributor

uklotzde Dec 29, 2021

I would prefer a proper fix here.

Contributor

uklotzde Dec 29, 2021

I am not going to merge a hack with a wrong comment.

Member Author

daschuer Dec 29, 2021

Why do you think the comment is wrong? Add least it is working.

But I don't mind to merge your ideas.
Can you issue a PR against my branch?

Thank you.

Member Author

daschuer Dec 29, 2021

The alternative would be to merge #4584 to this branch. This is IMHO the proper fix, but touches more code.

Contributor

uklotzde Dec 29, 2021

I haven't reviewed #4584 yet and I cannot promise to do it soon.

Contributor

uklotzde Dec 29, 2021

Moreover the build failed on Linux with Clang due to a wrong preprocessor directive. Fixed.

uklotzde added 3 commits

December 29, 2021 19:40


          Delete redundant std::move() invocation

0869a8a


          Keep the class invariants of SoundSourceProxy valid

9ba1c45

If the AudioSource is opened the internal pointer must be initialized
to ensure that it could be closed later!


          Fix preprocessor directive

bd12ecb

Fixes the build on Linux with CLang.

Contributor

uklotzde commented Dec 29, 2021 •

edited

Loading

~~This important bugfix should be mentioned in the CHANGELOG.~~ Ok, already labelled.


          Merge pull request #72 from uklotzde/lp1955331

8bb4d2c

lp1955331: Preserve class invariants of SoundSourceProxy

Member Author

daschuer commented Dec 30, 2021

We have got the confirmation of that it is working:
https://bugs.launchpad.net/mixxx/+bug/1955314

daschuer requested a review from uklotzde

December 30, 2021 12:16

daschuer added this to the 2.3.2 milestone

uklotzde approved these changes

View reviewed changes

Contributor

uklotzde left a comment

Thank you! LGTM

I'll merge to main

uklotzde merged commit 656dca1 into mixxxdj:2.3

daschuer deleted the lp1955331 branch

April 14, 2022 21:19

This was referenced Aug 23, 2022

File creation time is lost when exporting metadata (Windows) #10619

Closed

Metadata wirte to file tag fails sometimes in Win 10 #10620

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog major bug