upload: added forceMp4 flag #161
Conversation
Codecov Report
@@ Coverage Diff @@
## main #161 +/- ##
===================================================
- Coverage 10.87460% 10.84949% -0.02511%
===================================================
Files 14 14
Lines 2161 2166 +5
===================================================
Hits 235 235
- Misses 1909 1914 +5
Partials 17 17
Continue to review full report in Codecov by Sentry.
|
task/upload.go
Outdated
| generateMp4s := false | ||
| if tctx.OutputAsset.GenerateMp4s { | ||
| generateMp4s = true | ||
| } |
There was a problem hiding this comment.
any reason for the indirection?
| generateMp4s := false | |
| if tctx.OutputAsset.GenerateMp4s { | |
| generateMp4s = true | |
| } | |
| generateMp4s := tctx.OutputAsset.GenerateMp4s |
task/upload.go
Outdated
| @@ -156,7 +156,7 @@ func TaskTranscodeFile(tctx *TaskContext) (*TaskHandlerOutput, error) { | |||
| tctx: tctx, | |||
| inUrl: params.Input.URL, | |||
| getOutputLocations: func() ([]clients.OutputLocation, error) { | |||
| _, outputLocation, err := outputLocations(params.Storage.URL, params.Outputs.HLS.Path, false) | |||
| _, outputLocation, err := outputLocations(params.Storage.URL, params.Outputs.HLS.Path, false, false) | |||
There was a problem hiding this comment.
Would probably make sense to already add this to the transcode API as well. WDYT? cc @leszko
There was a problem hiding this comment.
Good question. I think we want to be able to generate MP4 files with the Transcode API, but I'm not sure if we want to always do it or introduce a flag for it. @ericxtang what do you think? Should we always generate MP4 outputs?
There was a problem hiding this comment.
As mentioned here by @ericxtang we also want this feature in Transcode API.
@gioelecerati could you add it here and also in the Transcode API in Studio?
There was a problem hiding this comment.
Sure! Still a staticMp4 flag in the params or would it be better to add a mp4 output in the outputs fields in the transcode request?
There was a problem hiding this comment.
Related discussion: livepeer/catalyst-api#518 (comment)
IMO we should not have it automatically generated for transcode API unless users explicitly request for it (the automatic generation, that is). It might be cleaner to wait until we have the Catalyst enum for this and then create a corresponding one on the Studio transcode-file API.
There was a problem hiding this comment.
Yes, we'll have a parameter in Transcode API to generate MP4. Here's the related Linear: https://linear.app/livepeer/project/mp4-in-transcode-api-708bdd704de5
task/upload.go
Outdated
| @@ -551,6 +555,7 @@ func outputLocations(outURL string, relativePath string, autoMp4s bool) ([]Outpu | |||
| SourceSegments: sourceSegments, | |||
| TranscodedSegments: true, | |||
| AutoMP4: autoMp4s, | |||
| GenerateMP4: generateMp4s, | |||
There was a problem hiding this comment.
Continuing the comment I made on the API, I think here and on the task object the field makes sense! Only on the assets that I think it is a bit weird.
e.g. GET /asset/:id => { ..., generateMp4s: true } what does it mean on a response for example? Since we're using a verb it'd have to be in the past to make more sense, but my suggestion is to not use a verb at all (on assets), using a "state description" instead.
gioelecerati
changed the title
livepeer/catalyst-api#518
livepeer/studio#1661