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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support transcode-file task #100

Merged
merged 15 commits into from
Dec 20, 2022
Merged

Support transcode-file task #100

merged 15 commits into from
Dec 20, 2022

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Dec 12, 2022

Add a new task handler TaskTranscodeFile. It has the same workflow as TaskUpload, but it's unrelated to any Studio asset, therefore the logic is slightly different.

Related PRs:


Differences between TaskTrancodeFile and TaskUpload:

  1. TaskUpload can read input public video URL from UploadedObjectKey, but TaskTranscodeFile reads it only from the given URL.
  2. TaskTranscodeFile allows to specify any bucket path as the output, while TaskUpload uses playbackId from the asset for the output path.
  3. TaskTrancodeFile finalizes its work when Catalyst Upload VOD pipeline is completed, while TaskTranscodeFile also verifies if the video is playable using playback URL.

This differences are expressed in the handleUploadVODParams strategy.


Additional Comments:

  • The related PR in livepeer/go-api-client needs to get merged before this one
  • I didn't add any unit tests, since there are currently almost no unit tests in task-runner 馃槺 I think we should definitely add them, but it would be separate activity / PR
  • I think we should definitely do some task-runner refactoring / clean-up (e.g. remove deprecated tasks, create tasks as structs, rather than just functions, etc.), but also I think it'll be a separate PR

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #100 (4f81aeb) into main (a6a00dd) will decrease coverage by 0.14461%.
The diff coverage is 0.00000%.

Impacted file tree graph

@@                Coverage Diff                @@
##               main       #100         +/-   ##
=================================================
- Coverage   9.18473%   9.04012%   -0.14461%     
=================================================
  Files            14         14                 
  Lines          1938       1969         +31     
=================================================
  Hits            178        178                 
- Misses         1745       1776         +31     
  Partials         15         15                 
Impacted Files Coverage 螖
task/runner.go 15.86538% <酶> (酶)
task/upload.go 0.00000% <0.00000%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update a6a00dd...4f81aeb. Read the comment docs.

Impacted Files Coverage 螖
task/runner.go 15.86538% <酶> (酶)
task/upload.go 0.00000% <0.00000%> (酶)

@leszko leszko marked this pull request as ready for review December 15, 2022 11:09
@leszko leszko requested a review from a team as a code owner December 15, 2022 11:09
@leszko leszko marked this pull request as draft December 15, 2022 12:52
@leszko leszko marked this pull request as ready for review December 15, 2022 13:55
Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

Very neat, I like that you were able to share most of the code with the Upload path.

Only thing I wasn't sure about was this bit in your PR description:

TaskTranscodeFile also verifies if the video is playable using playback URL

Where's that happening?

@leszko
Copy link
Contributor Author

leszko commented Dec 19, 2022

TaskTranscodeFile also verifies if the video is playable using playback URL

Where's that happening?

I might be wrong, but I think part of this code tries to download some files...

func complementCatalystPipeline(tctx *TaskContext, assetSpec api.AssetSpec, callback *clients.CatalystCallback) (*data.UploadTaskOutput, error) {

@leszko leszko mentioned this pull request Dec 19, 2022
4 tasks
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Clean implementation! LGTM, only some minor/optional nits :)

task/upload.go Outdated Show resolved Hide resolved
task/upload.go Outdated Show resolved Hide resolved
@yondonfu
Copy link
Member

yondonfu commented Dec 19, 2022

@leszko

TaskTrancodeFile finalizes its work when Catalyst Upload VOD pipeline is completed, while TaskTranscodeFile also verifies if the video is playable using playback URL.

I think there might be a typo here and this should read as:

"TaskTrancodeFile finalizes its work when Catalyst Upload VOD pipeline is completed, while TaskUpload also verifies if the video is playable using playback URL."

Right?

Also, for clarification, I don't think "verifies if the video is playable using playback URL" is quite right. The code that you linked in this comment is responsible for handling some additional operations after results are returned by Catalyst - for example, the complementCatalystPipeline() function will copy the source file from its URL into Studio's default bucket (i.e. GCP), probe the source file among other things. These are operations that ideally Catalyst itself can handle going forward, but until then task-runner is "complementing" the Catalyst functionality within this function at the end of the TaskUpload flow. A better way to think about the conclusion of the TaskUpload flow may be that additional operations are executed to complete asset creation (i.e. gather probing metadata) and these additional operations are not necessary for TaskTranscodeFile because the new task does not work with assets at all.

@leszko
Copy link
Contributor Author

leszko commented Dec 20, 2022

TaskTrancodeFile finalizes its work when Catalyst Upload VOD pipeline is completed, while TaskTranscodeFile also verifies if the video is playable using playback URL.

I think there might be a typo here and this should read as:

"TaskTrancodeFile finalizes its work when Catalyst Upload VOD pipeline is completed, while TaskUpload also verifies if the video is playable using playback URL."

Right?

Yes, you're right. Typing faster than thinking 馃檭

Also, for clarification, I don't think "verifies if the video is playable using playback URL" is quite right. The code that you linked in this comment is responsible for handling some additional operations after results are returned by Catalyst - for example, the complementCatalystPipeline() function will copy the source file from its URL into Studio's default bucket (i.e. GCP), probe the source file among other things. These are operations that ideally Catalyst itself can handle going forward, but until then task-runner is "complementing" the Catalyst functionality within this function at the end of the TaskUpload flow. A better way to think about the conclusion of the TaskUpload flow may be that additional operations are executed to complete asset creation (i.e. gather probing metadata) and these additional operations are not necessary for TaskTranscodeFile because the new task does not work with assets at all.

Yes, thanks for this clarification!

@leszko leszko merged commit ba9a880 into main Dec 20, 2022
@leszko leszko deleted the rafal/storj branch December 20, 2022 08:59
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

4 participants