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

Output rendition with dimensions == source #125

Merged
merged 4 commits into from
Nov 1, 2022
Merged

Conversation

thomshutt
Copy link
Contributor

@thomshutt thomshutt commented Nov 1, 2022

This resolves #124

Bitrate int32 `json:"bitrate,omitempty"`
FPS uint `json:"fps,omitempty"`
FPSDen uint `json:"fpsDen,omitempty"`
Width int64 `json:"width,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were using different types in different structs, so standardised on int64 / float64

Copy link
Contributor

Choose a reason for hiding this comment

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

Go is much better than c++ when handling int and uint arithmetic !

@@ -89,7 +89,7 @@ func TestRecordingCompleted(t *testing.T) {
require.NotNil(t, message.Success)
require.True(t, *message.Success)
require.GreaterOrEqual(t, message.Timestamp, testStartTime)
require.Less(t, message.Timestamp, testStartTime+2)
require.Less(t, message.Timestamp, testStartTime+100)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing changed here, but the timing was a bit too tight on CI and so I was seeing some failures

@thomshutt thomshutt requested review from AlexKordic and emranemran and removed request for AlexKordic November 1, 2022 11:00
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #125 (aa844a6) into main (c316c33) will increase coverage by 0.69791%.
The diff coverage is 62.50000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #125         +/-   ##
===================================================
+ Coverage   39.71261%   40.41052%   +0.69791%     
===================================================
  Files             24          24                 
  Lines           1531        1559         +28     
===================================================
+ Hits             608         630         +22     
- Misses           844         850          +6     
  Partials          79          79                 
Impacted Files Coverage Δ
clients/broadcaster.go 0.00000% <ø> (ø)
clients/callback_client.go 53.43511% <0.00000%> (-2.56489%) ⬇️
handlers/misttriggers/recording_end.go 20.30075% <0.00000%> (ø)
transcode/transcode.go 56.45161% <86.95652%> (+9.39279%) ⬆️

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 c316c33...aa844a6. Read the comment docs.

Impacted Files Coverage Δ
clients/broadcaster.go 0.00000% <ø> (ø)
clients/callback_client.go 53.43511% <0.00000%> (-2.56489%) ⬇️
handlers/misttriggers/recording_end.go 20.30075% <0.00000%> (ø)
transcode/transcode.go 56.45161% <86.95652%> (+9.39279%) ⬆️

Copy link
Contributor

@AlexKordic AlexKordic left a comment

Choose a reason for hiding this comment

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

Well Done!

Comment on lines +79 to +83
Name: "360p",
MediaData: []byte("pretend media data"),
},
{
Name: "super-high-def",
Name: "720p",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note for real broadcaster: name must start with letter otherwise transcode fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realise, good to know thanks!

@thomshutt thomshutt merged commit c44a001 into main Nov 1, 2022
@thomshutt thomshutt deleted the source-rendition-output branch November 1, 2022 11: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.

Top quality rendition should be equal dimensions to the source
2 participants