Skip to content

MP3 channel and decoding fix#568

Merged
daschuer merged 7 commits into
mixxxdj:masterfrom
uklotzde:MP3ChannelAndDecodingFix
Apr 25, 2015
Merged

MP3 channel and decoding fix#568
daschuer merged 7 commits into
mixxxdj:masterfrom
uklotzde:MP3ChannelAndDecodingFix

Conversation

@uklotzde
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde commented Apr 25, 2015

Fixes: https://bugs.launchpad.net/mixxx/+bug/1448224

SoundSourceMp3

  • Accept MP3 files with varying number of channels (1 or 2)
  • Last commit: Fix error handling for libMAD which was severely broken!

uklotzde added 6 commits April 25, 2015 13:11
@uklotzde
Fix handling of mono MP3 frames within a stereo file
6d9c259
@uklotzde
Abort loading of MP3 file on error before determining sampling rate
18d7ec3
@uklotzde
Limit the maximum number of channels in MP3 files to 2
4a2023a
@uklotzde
Fix debug assertion to accept MP3 files without any tags
26949dc
@uklotzde
Some minor code reorderings when loading MP3 files
1b07544
@uklotzde
Fix libMAD error handling for MP3 decoding
b7d9985 
The decoding functions do NOT return the actual error code!! Instead
the error code must be read from mad_stream if the decoding functions
returned != 0.

This caused some really ugly sounds at the beginning of some MP3 files,
because frames were not skipped as suppossed to be.
daschuer
daschuer reviewed Apr 25, 2015
View reviewed changes
Comment thread src/sources/soundsourcemp3.cpp Outdated
Copy link
Copy Markdown
Member

@daschuer daschuer Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves some more comments. I an not sure where, I am still trying to understand. Please correct me:
"Here we are, if we have decoded a mono sample. This can be either intended or a faulty frame"

Copy link
Copy Markdown
Member

@daschuer daschuer Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"And If we decode a mono file"

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 25, 2015

LGTM. Thank you.
Only some doku issues.

Will it simplify the code, if SoundSourceMp3 predicts to be always be stereo?
I am sure it is true anyway in almost all cases.

@uklotzde
Separate code paths for reading mono/stereo MP3 data
e003d06 
Slightly more code, but hopefully easier to understand.
@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 25, 2015

Restriction to stereo: It would eliminate the maximum channel calculation and 1 of 3 cases when reading sample data. I don't think that it's worth to alter the code again without any real issues, now that we have a general approach with lots of sanity checks.

If you think about an optimized mono path for audio processing in the future (I often switch the master output to mono) then this artificial restriction could even become counterproductive.

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 25, 2015

Thank you for the fix.

There are strange errors at Travis:

Reading tags from file "/home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav" of type "wav" 
Reading tags from file "/home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav" of type "wav" 
Reading tags from file "/home/travis/build/mixxxdj/mixxx/src/test/sine-30.wav" of type "wav" 
in ~EngineMaster() 
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

I have just build an test this version without such errors. So it is hopefully a server issue.

daschuer added a commit that referenced this pull request Apr 25, 2015
@daschuer
Merge pull request #568 from uklotzde/MP3ChannelAndDecodingFix
65e96f5 
MP3 channel and decoding fix
@daschuer daschuer merged commit 65e96f5 into mixxxdj:master Apr 25, 2015
@uklotzde
Copy link
Copy Markdown
Contributor Author

uklotzde commented Apr 25, 2015

I'm currently seeing test failures on 1.12 for EngineBufferE2ETest.ScratchTest. But on master everything is fine :)

@uklotzde uklotzde deleted the MP3ChannelAndDecodingFix branch May 6, 2015 22:17
@mixxxbot mixxxbot mentioned this pull request Aug 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@uklotzde @daschuer