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

server, net: Handle back compat for MP4. #1439

Merged
merged 2 commits into from
Apr 9, 2020
Merged

server, net: Handle back compat for MP4. #1439

merged 2 commits into from
Apr 9, 2020

Conversation

j0sh
Copy link
Collaborator

@j0sh j0sh commented Apr 4, 2020

What does this pull request do? Explain your changes. (required)
This is another approach to compatibility rather than a hard break via versioning as described in #1433 .

  • Makes MP4 requests fail out on orchestrators that don't support MP4 (or more generally, the FullProfile field for non-preset renditions introduced in Ar/handle custom bitrate ladder #1242 )

  • Maintains compatibility with orchestrators that can only process mpegts

Specific updates (required)

  • 5deedb6 Adds a new FullProfiles2 field to net.SegData
  • 103c540 Use the old FullProfiles field if the output format is only mpegts . This maintains compatibility with older orchestrators that only know mpegts.
  • 2212e9c Fails out with orchestrators that do not support the above two fields
  • Adds unit tests
  • 9dfcabf Update to generated protocol buffers, to be fixed up with bf41208

How did you test each of these updates (required)

  • Added unit tests
  • Manual testing with:
    • old B -> new O (mpegts only)
    • new B -> old O (one test for mpegts, another for mp4)
    • new B -> new O (one test for mpegts, another for mp4)

Does this pull request close any open issues?

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

@j0sh j0sh changed the title WIP: server, net: Handle back compat for MP4. server, net: Handle back compat for MP4. Apr 6, 2020
common/util.go Outdated Show resolved Hide resolved
@@ -663,7 +663,7 @@ func (rt *RemoteTranscoder) Transcode(job string, fname string, profiles []ffmpe
return nil, RemoteTranscoderFatalError{err}
}

fullProfiles, err := common.FFmpegProfiletoNetProfile(profiles)
fullProfiles, _, err := common.FFmpegProfiletoNetProfile(profiles)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assuming for now that standalone Ts will be upgraded in tandem with Os, so we aren't attempting to handle back compat here.

@j0sh j0sh marked this pull request as ready for review April 6, 2020 19:29
common/util.go Outdated Show resolved Hide resolved
net/lp_rpc.proto Outdated Show resolved Hide resolved
server/segment_rpc.go Outdated Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM :shipit: after a squash

j0sh added 2 commits April 8, 2020 21:45
This allows us to fail out quickly on orchestrators that do not
recognize the FullProfile field.
Only use this field for non-mpegts profiles in order to maintain
compatibility with older orchestrators. If an older orchestrator
receives a request for non-mpegts transcoding, it will fail out.
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