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

Use correct format for playing 24-bit files with op/oss #614

Merged
merged 1 commit into from Jan 28, 2017
Merged

Use correct format for playing 24-bit files with op/oss #614

merged 1 commit into from Jan 28, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jan 21, 2017

Adding support for AFMT_{S24,U24}_{LE,BE} will make the format
selection even more difficult to read, so while here replace it with a
simple lookup table.

Tested on FreeBSD. The symptoms of using the wrong format were
the same as with the sndio plugin (#613).

#define AFMT_S24_BE 0x00020000
#endif
#ifndef AFMT_U24_LE
#define AFMT_U24_LE 0x00040000
Copy link
Member

Choose a reason for hiding this comment

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

This seems rather dubious. Why was this constant S24_PACKED before?

Copy link
Author

Choose a reason for hiding this comment

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

I was under the impression that the constants are the same across different OSS implementation, but that's apparently not true. S24_PACKED isn't even defined on FreeBSD and U24_LE isn't defined in OSSv4.

I don't really understand why the constants need to be added here at all to be honest. I've amended the patch a little.

} else {
tmp = AFMT_U8;
found = 0;
for (i = 0; i < sizeof(oss_fmts)/sizeof(oss_fmts[0]); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

There is a macro for this in utils.h

@mahkoh
Copy link
Member

mahkoh commented Jan 28, 2017

This seems to remove support for OSSv3 which requires some justification.

@ghost
Copy link
Author

ghost commented Jan 28, 2017

That wasn't my intent. On what system do you see problems?

@mahkoh
Copy link
Member

mahkoh commented Jan 28, 2017

-/* defined only in OSSv4, but seem to work in OSSv3 (Linux) */

Adding support for AFMT_{S24,U24}_{LE,BE} will make the format
selection even more difficult to read, so while here replace it with a
simple lookup table.

Tested on FreeBSD.
@ghost
Copy link
Author

ghost commented Jan 28, 2017

I've re-added the constant-definining-block under an #ifdef __linux__, but I still don't understand where exactly the magic numbers come from.

@mahkoh
Copy link
Member

mahkoh commented Jan 28, 2017

I'll merge this for now. If anyone still uses OSS, we'll deal with breakage when it happens.

@mahkoh mahkoh merged commit 6a85680 into cmus:master Jan 28, 2017
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.

None yet

1 participant