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

Dont send videos with rotation to mist #553

Merged
merged 2 commits into from
Apr 3, 2023
Merged

Dont send videos with rotation to mist #553

merged 2 commits into from
Apr 3, 2023

Conversation

mjh1
Copy link
Contributor

@mjh1 mjh1 commented Mar 30, 2023

Rotation was found to be ignored by mist so this ensures we only send videos like that to mediaconvert

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #553 (ba07dbb) into main (86443b2) will increase coverage by 0.22413%.
The diff coverage is 100.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #553         +/-   ##
===================================================
+ Coverage   55.16202%   55.38615%   +0.22413%     
===================================================
  Files             49          49                 
  Lines           3981        4001         +20     
===================================================
+ Hits            2196        2216         +20     
  Misses          1602        1602                 
  Partials         183         183                 
Impacted Files Coverage Δ
video/profiles.go 71.05263% <ø> (ø)
pipeline/coordinator.go 81.38889% <100.00000%> (+0.36906%) ⬆️
video/probe.go 78.46154% <100.00000%> (+2.39316%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86443b2...ba07dbb. Read the comment docs.

Impacted Files Coverage Δ
video/profiles.go 71.05263% <ø> (ø)
pipeline/coordinator.go 81.38889% <100.00000%> (+0.36906%) ⬆️
video/probe.go 78.46154% <100.00000%> (+2.39316%) ⬆️

@@ -291,39 +291,6 @@ func Test_MP4OutDurationCheck(t *testing.T) {
}
}

func TestProbe(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this to probe_test.go, it was missed in a previous refactor

go.mod Outdated
@@ -174,3 +174,5 @@ require (
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

replace gopkg.in/vansante/go-ffprobe.v2 => github.com/mjh1/go-ffprobe v0.0.0-20230330150500-3b664b71952e
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you raise a PR against the upstream repo?

Copy link
Contributor Author

@mjh1 mjh1 Mar 31, 2023

Choose a reason for hiding this comment

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

not yet as i was planning on expanding it to include all the different possible side data fields. but through a bit of searching it looks like there may be too much to include in named struct fields :( can't find a definitive list of all variations.
Found this typescript version: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/ffprobe/index.d.ts

But then chatgpt says there are way more options ha (each with their own set of associated fields):
Screenshot 2023-03-31 at 12 24 06

So might have to be a map[string]interface{} kind of thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds like a good idea and then we can leave it to someone else to do the full list if needed

@mjh1 mjh1 merged commit 0356498 into main Apr 3, 2023
@mjh1 mjh1 deleted the mh/vid-rotation branch April 3, 2023 10:48
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.

2 participants