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

Non Integer Frame Rate Support #1474

Closed
2 of 3 tasks
j0sh opened this issue Apr 27, 2020 · 4 comments · Fixed by #1525
Closed
2 of 3 tasks

Non Integer Frame Rate Support #1474

j0sh opened this issue Apr 27, 2020 · 4 comments · Fixed by #1525
Assignees

Comments

@j0sh
Copy link
Collaborator

j0sh commented Apr 27, 2020

TODO:

  • Spec
  • Implementation for LPMS
  • Implementation for go-livepeer

Notes:

  • Configurable via auth webhook
  • Propagation through codebase via VideoProfile, similar to MP4
  • Add fps_den field (rational number representation)
  • Example: MP4 spec, MP4 implementation (1) (2) (3)
@iameli
Copy link
Contributor

iameli commented May 4, 2020

I'd like to put in a vote on the webhook spec for representing the fraction as a two-element tuple rather than an additional fps_den field.

{
  "profiles": [
    {
      "name": "test_stream_29.97fps",
      "width": 1280,
      "height": 720,
      "bitrate": 4000000,
      "fps": [30000, 1001]
    },
    {
      "name": "test_stream_30fps",
      "width": 1280,
      "height": 720,
      "bitrate": 4000000,
      "fps": 30
    }
  ]
}

Bit more terse and readable IMO.

@j0sh
Copy link
Collaborator Author

j0sh commented May 4, 2020

The trouble is, this makes it more difficult to statically define the JSON schema to generate de/serializers. In googling around how to do this for golang, the consensus is basically "this is bad practice; don't do it." Eg: https://medium.com/@nate510/dynamic-json-umarshalling-in-go-88095561d6a0

@jailuthra jailuthra self-assigned this May 7, 2020
@jailuthra
Copy link
Contributor

jailuthra commented May 19, 2020

Abstract

Currently go-livepeer supports transcoding videos to integer framerates. This is a proposal to add support for arbitrary non-integer framerates in the output video profiles.

Motivation

Some broadcasting standards (NTSC/ATSC) work on esoteric non-int framerates like 29.97 etc. As these standards are still widely used, users want the ability to transcode videos to non-int framerates through go-livepeer.

Proposed Solution

  • Accept non-integer framerates as a rational number (numerator/denominator) in go-livepeer.
  • Propogate the FPS value through the VideoProfile struct, pass it on as an AVRational to FFmpeg.
  • Ability to set non-int FPS through the auth webhook.

Implementation Tasks and Considerations

LPMS Changes

Cgo

FFmpeg uses a custom rolled rational datatype (AVRational) to support non-integer framerates. The Cgo code in LPMS represents fps as an AVRational output parameter, so no changes needed here.

VideoProfile

The VideoProfile struct uses a Framerate uint field for framerate. This struct can be extended with another FramerateDen uint optional field for the denominator, treating it as 1 if unitialized by the user.

Transcoding

The transcoder module initializes the FPS ffmpeg filter using a filtergraph string with a hardcoded denominator. We can change it to use the denominator field we added in VideoProfile.

Here we also set the output parameters passed to the Cgo module which represent fps as a rational datatype. Currently we're hardcoding the denominator, but can trivially fix it to use our new field.

Protobuf

Modify the VideoProfile protobuf struct to add a new unsigned int field for the FPS denominator. The changes will mirror those done in the LPMS VideoProfile struct.

Webhook Configurability

We want to configure the output videoprofile using auth webhook as well. To accomplish this we can modify the webhook json response to send the FPS denominator as a separate field.

Pixel calculations and other uses

The FPS value is used at some other places like counting the output pixels and cost calcuations. We can find all such uses and ensure the correct value (num/den) is used everywhere.

We'll also need to find all the places the VideoProfile structure is converted between the Go/Json/Protobuf types and ensure that the denominator is being moved as well.

Testing Tasks and Considerations

Add unit tests to fully cover the new code paths added to support non-integer framerates.

Known Unknowns

Alternatives

We could've modified the VideoProfile structure by extending the existing Framerate uint field to Framerate big.Rat instead of adding a new field.

It might look much cleaner but we would've lost backwards compatibility, needed changes almost everywhere the structure is used and also would have had a hard time supporting the protobuf/json variants.

Additional Context

@j0sh
Copy link
Collaborator Author

j0sh commented May 19, 2020

This is great.

This struct can be extended with another FramerateDen uint optional field for the denominator, treating it as 1 if unitialized by the user.

Yep. Perhaps this only needs to occur at the very last step prior to invoking the Cgo transcoder, eg somewhere in ffmpeg.go . This allows us to safely propagate an uninitialized / zero-value FramerateDen throughout the codebase without being too concerned about preconditions on the FramerateDen value.

We'll also need to find all the places the VideoProfile structure is converted between the Go/Json/Protobuf types

This PR might make things easier, since it will accommodate standalone T and combined OT with the same code: 0e1902b . Will wrap it up soon so you can build off it.

For the webhook: here is the response struct and the conversion procedure from JSON to VideoProfile.

We could've modified the VideoProfile structure by extending the existing Framerate uint field to Framerate big.Rat instead of adding a new field. ... It might look much cleaner but we would've lost backwards compatibility

Technically, since the ffmpeg.VideoProfile is a local struct, I think we can modify it at-will, as long as we also adapt the de/serialization processes to fit the new scheme. However, I do agree that in general, this type of change is not really preferable anyway, for many of the reasons you mentioned.

Related, another concern that wasn't discussed here is back compatibility. At the outset, most orchestrators will not update immediately. So it's worth considering this: identify whether fractional framerates are required for the current job, and to identify (or error out on) orchestrators that haven't yet upgraded to support the feature. This allows us to use older orchestrators for jobs that don't require fractional framerates (which will be the majority of jobs).

Ideally, this is something that's taken care of by a capability discovery scheme, but it is possible that capability discovery won't be ready by the time this is complete. An interim solution could follow the pattern in #1439 ; eg adding a FullProfiles3 that's only set if fractional frame rates are specified in the output. Orchestrators that don't recognize the field will error 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 a pull request may close this issue.

3 participants