spatialaudio / python-sounddevice

:sound: Play and Record Sound with Python :snake:
https://python-sounddevice.readthedocs.io/
MIT License
980 stars 145 forks source link

fix decode error #491

Closed shoucandanghehe closed 5 months ago

shoucandanghehe commented 10 months ago

I tried to introduce the locale package to get the default encoding of the user's system. And I think there shouldn't be any encoding types other than those two. But I can't be sure, so I just output bytes. that way there won't be any more errors caused by decoding failures.

close #490

mgeier commented 10 months ago

Thanks for this PR!

Why are you removing the mbcs stuff? Is it less correct than your implementation?

And I thought this only affects ASIO? Why are you applying it to all host APIs?

shoucandanghehe commented 10 months ago

Why are you removing the mbcs stuff? Is it less correct than your implementation?

As far as I know, the default encoding for some regions of Windows is UTF-16, and mbcs can't decode it, so I tried to get the default encoding for the user's system and use it.

And I thought this only affects ASIO? Why are you applying it to all host APIs?

I don't think judging the host API is necessary, if an error occurs decoding in UTF-8, then just try to decode it using another encoding. If you think it will definitely be UTF-8 encoded, then it won't even get to that if. If it's not UTF-8, the worst that can happen is that an exception is thrown again, (if we don't use repr) but if it's the default encoding, we're good to go.

mgeier commented 9 months ago

As far as I know, the default encoding for some regions of Windows is UTF-16

This may well be, but do you know for sure whether each single host API actually uses the default encoding? Did you test this?

I don't think judging the host API is necessary

It may not be, but I don't know that for sure.

I only know that this was tested on multiple systems in #72.

And AFAIU, you only saw the problem with ASIO and not with any other host API, right?

The problem is that I cannot test any of this myself and I have to rely on reports by other people.

shoucandanghehe commented 9 months ago

This may well be, but do you know for sure whether each single host API actually uses the default encoding? Did you test this?

I certainly can't test all versions of Windows, so I added the fallback just so I can continue to use it if the name decoding fails.

PS. I think it's very frustrating to have an entire program unavailable because the name of some mysterious device was decoded incorrectly.

And AFAIU, you only saw the problem with ASIO and not with any other host API, right?

Yes, but I don't think it's going to get any worse, for the reasons I stated above, utf-8 is still the first-order decoding scheme.

mgeier commented 9 months ago

This may well be, but do you know for sure whether each single host API actually uses the default encoding? Did you test this?

I certainly can't test all versions of Windows, so I added the fallback just so I can continue to use it if the name decoding fails.

Yeah, but maybe this leads to worse behavior (unreadable device names) in some cases that did work before. We don't know that, but I would like to not risk that.

PS. I think it's very frustrating to have an entire program unavailable because the name of some mysterious device was decoded incorrectly.

I agree. That's why it would be great to fix the error with ASIO that you actually witnessed.

However, it would also be frustrating if the program does not crash but instead some device names contain gibberish. Using the suggested fallback too often might lead to that problem.

In either case (crash or gibberish), I have to rely on user feedback (like yours) to be able to fix the problem. I just think that a crash is a better incentive for people to report a problem.

And I'm open to fix every single problem that comes up. I just want to be cautious not to re-introduce errors on systems that have been working correctly before.

And AFAIU, you only saw the problem with ASIO and not with any other host API, right?

Yes, but I don't think it's going to get any worse, [...]

That's the problem, I don't know that. Maybe it gets worse, maybe it doesn't. That's why I would like to keep the behavior that has actually be tested and confirmed to work correctly and only fix the behavior that has actually been observed to be broken.

pep8speaks commented 9 months ago

Hello @shoucandanghehe! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 576:80: E501 line too long (81 > 79 characters)

mgeier commented 6 months ago

I have created an alternative to this PR: #512.

@shoucandanghehe Can you please check if that also fixes your problem?

shoucandanghehe commented 6 months ago

@shoucandanghehe Can you please check if that also fixes your problem?

Actually, I don't know what happened to my driver, anyway that device disappeared😇

mgeier commented 5 months ago

I have merged #512 instead.