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

check sample rate and lower if too high #2986

Merged
merged 3 commits into from
Jul 15, 2020
Merged

Conversation

Death916
Copy link
Contributor

New pull request with same commit as this one #2933

Just had to clean up some git stuff.

Issue #2906

@eukreign eukreign added area: blobs/files type: improvement Existing (or partially existing) functionality needs to be changed labels Jun 27, 2020
log.debug(" Detected audio codec is %s", codec)
if not {"aac", "mp3", "flac", "vorbis", "opus"}.intersection(codec.split(",")):
return "Audio codec is not in the approved list of AAC, FLAC, MP3, Vorbis, and Opus. " \
f"Actual: {codec} [{stream['codec_long_name']}]"
if sample_rate > "48000":
Copy link
Member

Choose a reason for hiding this comment

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

What are the possible other sample_rate values? This comparison will produce an incorrect result if the length of the string is different even if one rate is higher than the other rate... since it's doing string comparisons, not integer comparisons.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Death916 can you address the above? Maybe we can make the check more robust in case it does not exist.

Copy link
Contributor Author

@Death916 Death916 Jun 30, 2020

Choose a reason for hiding this comment

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

@tzarebczan @eukreign
Sorry was busy for a bit. I changed the checks to int on my local now. i originally just figured it wouldn't matter since the values were already strings from the json.
What do you mean by it does not exist though?
I was mostly just trying to address the issue mentioned specifically for this PR, but if you could elaborate on what you mean it shouldn't be a problem to do. Kind of late here so I could be missing something obvious. Do you mean if it cant find a sample rate to just give it one that's acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

@Death916 doing string comparisons leads to unexpected behavior:

>>> "50" > "48000"
True
>>> "3000000000" > "48000"
False
>>> "a" > "48000"
True

what are the possible values that sample_rate can be? can it be an empty string? can it be something other than an integer (perhaps a float? what else?)?
once we have a better idea of possible range of values then at least for the integer/float case the two sides of the > equality need to be converted to integers or floats.

@lbry-bot lbry-bot assigned eukreign and unassigned eukreign Jun 27, 2020
@eukreign eukreign assigned Death916 and unassigned eukreign Jun 27, 2020
@lbry-bot lbry-bot assigned eukreign and unassigned Death916 and eukreign Jun 30, 2020
@eukreign eukreign assigned Death916 and unassigned eukreign Jul 1, 2020
@eukreign eukreign merged commit 383f0ce into lbryio:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: blobs/files type: improvement Existing (or partially existing) functionality needs to be changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants