Skip to content
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.

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

lp1766834: Detect M4A decoding errors on Windows #1646

Merged
merged 2 commits into from
May 13, 2018
Merged

lp1766834: Detect M4A decoding errors on Windows #1646

merged 2 commits into from
May 13, 2018

Conversation

uklotzde
Copy link
Contributor

https://bugs.launchpad.net/mixxx/+bug/1766834

I don't know how frequently the decoder gets out of sync while reading. Currently we have only a single
example provided by Sean. But it is definitely a critical issue if playback or analysis stops unexpectedly!!

@uklotzde uklotzde added this to the 2.1.1 milestone Apr 25, 2018
@uklotzde uklotzde changed the title lp1766834: Fix M4A decoding errors on Windows [WiP] lp1766834: Fix M4A decoding errors on Windows Apr 25, 2018
@uklotzde
Copy link
Contributor Author

Still doesn't work as expected, need more testing. Rounding errors seem to accumulate while reading. i.e. the reader is reporting inaccurate positions.

...and avoid to update if not necessary to prevent rounding errors.
Just log any suspicious values.
@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 25, 2018

Finally I understood that the example file is actually corrupt and IMFSourceReader is reporting wrong read positions! The implementation is now more robust and logs those errors, many errors.

I'm won't touch this again without any important reasons to do so. Lots of wasted time hunting ghost bugs, frustrating. ClosedFinished.

@uklotzde uklotzde changed the title [WiP] lp1766834: Fix M4A decoding errors on Windows lp1766834: Fix M4A decoding errors on Windows Apr 25, 2018
@Pegasus-RPG
Copy link
Member

I'm confused. What does this PR end up doing? More error-checking such that corrupt files are handled more gracefully?

@uklotzde
Copy link
Contributor Author

  • Even the previous rounding code should have worked for regular files. Now the conversions are easier to grasp.
  • The reading code is more robust if wrong stream positions are reported and ignores them if possible.

In the end this might not even be a bug in our code, although improving robustness for corrupt files is at least one actual benefit. Otherwise the effort I spent on this would be totally lost.

@uklotzde
Copy link
Contributor Author

Reading should not abort without an indication why. Now you get some nice logs:

SoundSourceMediaFoundation - streamPos [100 ns] = 1625170 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 7168 actual = 7167
SoundSourceMediaFoundation - streamPos [100 ns] = 1857369 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 8192 actual = 8191
SoundSourceMediaFoundation - streamPos [100 ns] = 2089569 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 9216 actual = 9215
SoundSourceMediaFoundation - streamPos [100 ns] = 2321768 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 10240 actual = 10239
SoundSourceMediaFoundation - streamPos [100 ns] = 2553968 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11264 actual = 11263
SoundSourceMediaFoundation - streamPos [100 ns] = 2786167 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 12288 actual = 12287
SoundSourceMediaFoundation - streamPos [100 ns] = 3018367 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 13312 actual = 13311
SoundSourceMediaFoundation - streamPos [100 ns] = 3250566 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 14336 actual = 14335
SoundSourceMediaFoundation - streamPos [100 ns] = 3482766 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 15360 actual = 15359
SoundSourceMediaFoundation - streamPos [100 ns] = 3714965 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 16384 actual = 16383
...
SoundSourceMediaFoundation - streamPos [100 ns] = 2528419047 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11150336 actual = 11150328
SoundSourceMediaFoundation - streamPos [100 ns] = 2528651247 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11151360 actual = 11151352
SoundSourceMediaFoundation - streamPos [100 ns] = 2528883446 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11152384 actual = 11152376
SoundSourceMediaFoundation - streamPos [100 ns] = 2529115646 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11153408 actual = 11153400
SoundSourceMediaFoundation - streamPos [100 ns] = 2529347845 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11154432 actual = 11154424
SoundSourceMediaFoundation - streamPos [100 ns] = 2529580045 , sampleRate [Hz] = 44100
SoundSourceMediaFoundation - Stream position (in sample frames) while reading is inaccurate: expected = 11155456 actual = 11155448

@uklotzde
Copy link
Contributor Author

The deviations vary gradually, up to > 20 sample frames at some point.

@uklotzde uklotzde changed the title lp1766834: Fix M4A decoding errors on Windows lp1766834: Detect M4A decoding errors on Windows Apr 25, 2018
@Pegasus-RPG
Copy link
Member

Knowing why a problem is happening is much better in my view. I'm in favor of merging this.

@uklotzde
Copy link
Contributor Author

Tested this with some valid M4A files. All reported stream positions are accurate, no log spam.

@daschuer
Copy link
Member

LGTM. Thank you for nailing this down. Ready for merge.

@daschuer
Copy link
Member

Ready for merge?

@daschuer
Copy link
Member

@Pegasus-RPG merge?

@daschuer
Copy link
Member

@Pegasus-RPG merge?

Since we get no answer I will merge it now. If there are still pending issues we can fix them later.

@daschuer daschuer merged commit 5c27fd5 into mixxxdj:2.1 May 13, 2018
@uklotzde uklotzde deleted the lp1766834_windows_m4a_decoding_errors branch May 14, 2018 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants