FEATURE: Add support for aws MediaConvert #33092
Conversation
d87e737 to
b806554
Compare
In an upcoming pr #33092 that adds support for aws-sdk-mediaconvert it requires that we bump aws-sdk-core to 3.225.0 ``` + aws-sdk-mediaconvert (1.160.0) + aws-sdk-core (~> 3, >= 3.225.0) ``` but in doing so there are apparently some api changes and it requires changes in core to support the new version. I thought it would best to break out these changes in a separate PR as to not muddy up the mediaconvert pr that is already quite large.
In an upcoming pr #33092 that adds support for aws-sdk-mediaconvert it requires that we bump aws-sdk-core to 3.225.0 ``` + aws-sdk-mediaconvert (1.160.0) + aws-sdk-core (~> 3, >= 3.225.0) ``` but in doing so there are apparently some api changes and it requires changes in core to support the new version. I thought it would best to break out these changes in a separate PR as to not muddy up the mediaconvert pr that is already quite large.
In an upcoming pr #33092 that adds support for aws-sdk-mediaconvert it requires that we bump aws-sdk-core to 3.225.0 ``` + aws-sdk-mediaconvert (1.160.0) + aws-sdk-core (~> 3, >= 3.225.0) ``` but in doing so there are apparently some api changes and it requires changes in core to support the new version. I thought it would best to break out these changes in a separate PR as to not muddy up the mediaconvert pr that is already quite large.
e2c91ae to
5b4979f
Compare
| @@ -115,6 +116,11 @@ def store_file(file, path, opts = {}) | |||
| path, etag = s3_helper.upload(file, path, options) | |||
| end | |||
|
|
|||
| if opts[:upload_id] && FileHelper.is_supported_video?(opts[:filename]) && | |||
There was a problem hiding this comment.
This is where we trigger the video conversion job. It will only fire after it detects a video file being uploaded to s3 and the video conversion site setting is enabled.
|
|
||
| begin | ||
| response = | ||
| mediaconvert_client.create_job( |
There was a problem hiding this comment.
This is where we tell media convert to what video file in our existing s3 bucket to convert and where we want to store the output.
| if upload && optimized_video = OptimizedVideo.find_by(upload_id: upload.id) | ||
| # Only update if the URL is different | ||
| if container["data-video-src"] != optimized_video.url | ||
| container["data-original-video-src"] = container["data-video-src"] unless container[ |
There was a problem hiding this comment.
We add the original url back in here even though the video tag doesn't use it, this keeps the original upload from being cleaned up I believe.
There was a problem hiding this comment.
This looks great to me!
I didn't test this locally, but the overall logic and setup make sense. Only left a few minor comments regarding verbose logging.
@martin-brennan @tgxworld is this in good shape from your point of view?
| when :error | ||
| Rails.logger.error( | ||
| "Video conversion job failed for upload ID #{upload.id} and job ID #{args[:job_id]}", | ||
| ) |
There was a problem hiding this comment.
It may be useful to have a verbose_video_conversion_logging site setting in place and only output these logging entries when it's enabled. Could be default enabled for the initial PR, and then once kinks are ironed out, we switch the default to disabled.
When enabled this will convert uploaded videos to a standard format that should be playable on all devices and browsers. The goal of this feature is to prevent codec playback issues that sometimes can occur with video uploads.
Apparently with the new version of aws-sdk-core which mediaconvert requires a
hash ({}) is required for the object delete call.
…ing Discourse.warn_exception
Co-authored-by: Martin Brennan <martin@discourse.org>
Co-authored-by: Martin Brennan <martin@discourse.org>
75d4157 to
9b69f5f
Compare
|
|
||
| begin | ||
| new_sha1 = SecureRandom.hex(20) | ||
| output_path = "original/1X/#{new_sha1}" |
There was a problem hiding this comment.
It seems like we have some logic to derive the path for uploads in FileStore::BaseStore
discourse/lib/file_store/base_store.rb
Line 157 in ebf709f
Generally, I'm wondering if we can lean on the logic in UploadCreator here instead instead of duplicating some of the logic in this adapter.
When enabled this will convert uploaded videos to a standard format that should
be playable on all devices and browsers.
The goal of this feature is to prevent codec playback issues that sometimes can
occur with video uploads.
It uses an adapter pattern, so that other services for video conversion could be easily added in the future.