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

upload: generate dtsh before segmenting input file #177

Merged
merged 4 commits into from Nov 16, 2022

Conversation

emranemran
Copy link
Collaborator

@emranemran emranemran commented Nov 16, 2022

After a push request is sent, Mist attempt to read the input file, generate the dtsh header if there is none, and then begin segmenting the file. For certain files (typically large in size), the segmenting step would fail and generate an single empty 0.ts segment before bailing.

This change attempts to workaround this raciness within Mist by attempting to create the dtsh using an out-of-band call to MistInMP4 before segmenting begins.

Note: requires fix from mist repo for MistInMP4 to work as expected:
DDVTECH/mistserver#131

After a push request is sent, Mist attempt to read the input file,
generate the dtsh header if there is none, and then begin segmenting the
fil. For certain files (typically large in size), the segmenting step
would fail and generate an single empty 0.ts segment before bailing.

This change attempts to workaround this raciness within Mist by attempting
to create the dtsh using an out-of-band call to MistInMP4 before segmenting
begins.
return err
}
go func() {
if err := headerPrepare.Wait(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do the Wait() in an async function? Don't we need to wait for it to finish to get the benefits of the DTSH file being there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! My mistake from time when dtsh was being created after transcode response

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, good catch - missed this as well.

Comment on lines 156 to 165
// prepare .dtsh headers for all rendition playlists
for _, output := range outputs {
// output is multivariant playlist
err := createDtsh(requestID, output.Manifest)
if err != nil {
// should not block the ingestion flow or make it fail on error.
log.LogError(requestID, "master createDtsh() failed", err, "destination", output.Manifest)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We still need this for mist playback. Am i missing some info?

Copy link
Contributor

Choose a reason for hiding this comment

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

For mist playback we need dtsh of multivariant playlist, rendition's dtsh is not used in mistserver playback

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: MistInHLS needs to be fixed - dtsh for playlist is still broken which is why i removed it originally. Following up here: #178

Comment on lines 219 to 223
go func() {
if err := headerPrepare.Wait(); err != nil {
log.LogError(requestID, "createDtsh return code", err, "destination", destination)
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
go func() {
if err := headerPrepare.Wait(); err != nil {
log.LogError(requestID, "createDtsh return code", err, "destination", destination)
}
}()
if err := headerPrepare.Wait(); err != nil {
log.LogError(requestID, "createDtsh return code", err, "destination", destination)
}

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #177 (40bfc7f) into main (4ad9a0b) will decrease coverage by 0.87132%.
The diff coverage is 40.50633%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##                main        #177         +/-   ##
===================================================
- Coverage   48.36521%   47.49389%   -0.87132%     
===================================================
  Files             24          24                 
  Lines           1621        1636         +15     
===================================================
- Hits             784         777          -7     
- Misses           741         765         +24     
+ Partials          96          94          -2     
Impacted Files Coverage Δ
clients/mist_client_mock.go 0.00000% <0.00000%> (ø)
handlers/misttriggers/recording_end.go 18.75000% <0.00000%> (-5.49242%) ⬇️
handlers/upload.go 37.85714% <32.72727%> (-8.17461%) ⬇️
clients/mist_client.go 54.11765% <70.00000%> (+1.35169%) ⬆️
transcode/transcode_jobs.go 89.06250% <0.00000%> (-4.68750%) ⬇️

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 4ad9a0b...40bfc7f. Read the comment docs.

Impacted Files Coverage Δ
clients/mist_client_mock.go 0.00000% <0.00000%> (ø)
handlers/misttriggers/recording_end.go 18.75000% <0.00000%> (-5.49242%) ⬇️
handlers/upload.go 37.85714% <32.72727%> (-8.17461%) ⬇️
clients/mist_client.go 54.11765% <70.00000%> (+1.35169%) ⬆️
transcode/transcode_jobs.go 89.06250% <0.00000%> (-4.68750%) ⬇️

Comment on lines +126 to +128
// Once we're happy with the request, do the rest of the Segmenting stage asynchronously to allow us to
// from the API call and free up the HTTP connection
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart choice

@AlexKordic AlexKordic self-requested a review November 16, 2022 11:59
@thomshutt thomshutt merged commit aff7603 into main Nov 16, 2022
@emranemran emranemran deleted the emran/create-dtsh-before-segmenting branch November 16, 2022 17:46
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

3 participants