-
Notifications
You must be signed in to change notification settings - Fork 0
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
Transcode VBR mp3 files to CBR #1162
Conversation
It does lead me to wonder if we should just always transcode, rather than back and forth validating and conditionally fixing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - couple of small ideas
@@ -25,6 +25,7 @@ def result | |||
scope :copy_image, -> { where(type: "Tasks::CopyImageTask") } | |||
scope :bad_audio_duration, -> { where("result ~ '\"DurationDiscrepancy\":([5-9]\\d[1-9]|[6-9]\\d{2}|[1-9]\d{3})'") } | |||
scope :bad_audio_bytes, -> { where("result ~ '\"UnidentifiedBytes\":[1-9]'") } | |||
scope :bad_audio_vbr, -> { where("result ~ '\"VariableBitrate\":true'") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the porter change to add this, smart.
@@ -55,6 +56,10 @@ def bad_audio_bytes? | |||
porter_callback_inspect.dig(:Audio, :UnidentifiedBytes).to_i > 0 | |||
end | |||
|
|||
def bad_audio_vbr? | |||
!!porter_callback_inspect.dig(:Audio, :VariableBitrate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we indifferent access
the callbacks? nice
+1 I'm a big fan of !!
for bools
app/models/tasks/copy_media_task.rb
Outdated
@@ -58,7 +58,7 @@ def update_media_resource | |||
|
|||
media_resource.save! | |||
|
|||
if media_resource.status_complete? && (bad_audio_duration? || bad_audio_bytes?) | |||
if media_resource.status_complete? && (bad_audio_duration? || bad_audio_bytes? || bad_audio_vbr?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only place we use these 3 bad_audio_*?
methods, maybe combine into a single check, now that there are 4 conditions chained here? Purely aesthetic, this is fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
@@ -81,6 +85,16 @@ def slice_media! | |||
end | |||
end | |||
|
|||
def next_highest_bitrate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
Fixes #1109.
Requires PRX/Porter#164.
When a VBR file is uploaded, transcode it to CBR by running it back through Porter/ffmpeg.
Can be a bit slow, but better than having a misleading segmenter wavform.