proposal: add new refactored bitmovin provider using new sdk #210

zsiec · 2019-06-14T22:54:16Z

We're opening this PR as a draft and have yet to write tests. We are looking for feedback from the community on this approach. We've moved the Bitmovin provider from a single file to a new structure we think is more suited to extend, test, and maintain.

Our general approach here was to (1) abstract away the details of managing presets from the global scope and instead localize configurations for specific codecs/formats in their own files, with their logic accessible via interfaces, (2) do the same for generation of output "container"s, and (3) follow a similar model for enriching the JobStatus report with output information.

We are using the new Bitmovin Go SDK found here: https://github.com/bitmovin/bitmovin-api-sdk-go - any help from the folks at Bitmovin to CR this and ensure we're using this alpha-version SDK as intended would be very welcome.

codecov-io · 2019-06-15T00:42:03Z

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files          29       29           
  Lines        2914     2914           
=======================================
  Hits         2291     2291           
  Misses        372      372           
  Partials      251      251

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 986b203...0e26b5e. Read the comment docs.

zsiec · 2019-06-15T20:10:43Z

There is currently one known issue with the new SDK, filed here: bitmovin/bitmovin-api-sdk-go#1

jonathanhperry

Overall all the object usage looks correct

jonathanhperry · 2019-06-17T22:31:20Z

main.go

@@ -7,6 +7,7 @@ import (
 	"github.com/NYTimes/gizmo/server"
 	"github.com/NYTimes/video-transcoding-api/config"
 	_ "github.com/NYTimes/video-transcoding-api/provider/bitmovin"
+	_ "github.com/NYTimes/video-transcoding-api/provider/bitmovinnewsdk"


Would perhaps suggest using bitmovinopenapi if a new provider name is required

yup, makes sense, the name I chose was meant to be a placeholder until there was clarity on if this replaced the current provider or will be available in addition to it

Is this using a different API or just a different SDK?

If it's a different SDK or a new API that's already GA, let's have this replace the old provider 😁

It's the same API, but a new SDK (although the SDK is still in alpha)

That should be fine, let's do it! 😬

jonathanhperry · 2019-06-17T22:37:41Z

provider/bitmovinnewsdk/configuration/codec/h264.go

+		if err != nil {
+			return model.H264VideoConfiguration{}, err
+		}
+		cfg.MaxGop = gopSize // TODO: investigate this to ensure it respects preset gop mode and size


MaxGop is correct to use when defining the GOP in frames

jonathanhperry · 2019-06-17T22:47:58Z

provider/bitmovinnewsdk/bitmovin.go

+		vodHLSManifests = []model.ManifestResource{{ManifestId: manifestID}}
+	}
+
+	encResp, err := p.api.Encoding.Encodings.Start(enc.Id, model.StartEncodingRequest{VodHlsManifests: vodHLSManifests})


👍 for using the VodHlsManifests to start the HLS creation

the 👍 belongs to @jamescyeh as this is based off code originally written by him :)

jonathanhperry · 2019-06-17T22:54:52Z

provider/bitmovinnewsdk/bitmovin.go

+		return nil, errBitmovinInvalidConfig
+	}
+
+	api, err := bitmovin.NewBitmovinApi(func(apiClient *common.ApiClient) {


I'd recommend adding support for passing in an organization id. This enables a customer to use their personal API key to kick off encodes, but the encodes themselves are stored within an Organization's context, which allows colleagues to see the same encodes without logging into your personal user.

e.g.

apiClient.TenantOrgId = cfg.Bitmovin.OrganizationId

This is optional and can be left undefined.

Good to know, I'll add this.

jonathanhperry · 2019-06-17T22:58:39Z

provider/bitmovinnewsdk/configuration/codec/h264.go

+}
+
+func h264ConfigFrom(preset db.Preset, customData *map[string]map[string]interface{}) (model.H264VideoConfiguration, error) {
+	cfg := model.H264VideoConfiguration{}


We've added support for Preset Configurations that can be set on a codec. These are presets to set low-level codec properties (reference frames, b frames, etc) based on Bitmovin's recommendations. Preset enum: https://github.com/bitmovin/bitmovin-api-sdk-go/blob/master/model/preset_configuration.go and their definitions: https://bitmovin.com/docs/encoding/tutorials/how-to-optimize-your-h264-codec-configuration-for-different-use-cases.

May be useful to expose this in a job configuration.

Those look useful, I'll have to check to see how that best fits into the project structure as it sits today, and see what kinds of options are best for exposing special preset functionality provided only in a single or handful of providers.

thinking out loud, I kind of see this happening more often as we add support for Per-title Encoding, 10 bits, etc. I wonder if we should have something like a providerConfig being carried around for the specific parameters of a provider so when triggering a job we could validate if the providerConfig is supported or not.

Yeah that falls in line with my thinking as well. We already have that Capabilities interface function, that idea could be extended and given more thought.

jamescyeh · 2019-06-18T21:07:46Z

provider/bitmovinnewsdk/configuration/codec/aac.go

+	return model.AacAudioConfiguration{
+		Name:    fmt.Sprintf("aac_%s_%.0f", bitrate, defaultAACSampleRate),
+		Bitrate: &convertedBitrate,
+		Rate:    floatToPtr(defaultAACSampleRate),


From Bitmovin's side, are there any plans to override the JSON marshalling of each of the models, bypassing any confusion of zero values, and things getting ignored? With code generation, it may be something that could be relatively easy.

fsouza

Sharing some notes, I still want to take another look and may have more notes later today or tomorrow 😬

Thanks for working on this! :D

fsouza · 2019-06-18T18:07:46Z

main.go

@@ -7,6 +7,7 @@ import (
 	"github.com/NYTimes/gizmo/server"
 	"github.com/NYTimes/video-transcoding-api/config"
 	_ "github.com/NYTimes/video-transcoding-api/provider/bitmovin"
+	_ "github.com/NYTimes/video-transcoding-api/provider/bitmovinnewsdk"


Is this using a different API or just a different SDK?

If it's a different SDK or a new API that's already GA, let's have this replace the old provider 😁

fsouza · 2019-06-18T18:10:06Z

provider/bitmovinnewsdk/bitmovin.go

+)
+
+// Just to double check the interface is properly implemented
+var _ provider.TranscodingProvider = (*bitmovinProvider)(nil)


Let's move this to a test once we have a test suite, though since the factory returns *bitmovinProvider + we're registering the factory with provider.Register, this check isn't strictly necessary.

sounds good, will remove

fsouza · 2019-06-18T19:14:53Z

provider/bitmovinnewsdk/configuration/codec/vorbis.go

+
+const (
+	defaultVorbisSampleRate float64 = 48000
+)


Minor: since there's only one constant here, let's not use a block. Also, the type is not needed:

const defaultVorbisSampleRate = 48000

+1 I'll update

provider/bitmovinnewsdk/configuration/codec/h264.go

fsouza

This is looking good. Thank you very much for splitting it into multiple files. I don't know how I feel about having multiple packages though, could we have everything in a single package?

Or, if we chose to go the multi-packages route, could we make the helper packages internal? (I've been thinking about making provider packages internal too, just to guarantee no one can depend directly on them).

What do you think?

fsouza · 2019-06-24T14:48:33Z

provider/bitmovinnewsdk/types/custom_data.go

+import "errors"
+
+// CustomData holds data to set on encodings and configurations to hold relationship state
+type CustomData *map[string]map[string]interface{}


Oh just noticed this. Does it need to be a pointer?

Suggested change

type CustomData *map[string]map[string]interface{}

type CustomData map[string]map[string]interface{}

Or could we have some kind of structured data on our side that then gets marshaled into a JSON-object that looks arbitrary in the bitmovin API? Something like:

type CustomData struct { Audio struct { ID string `json:"id"` } `json:"audio"` Container struct { Name string `json:"name"` } `json:"container"` Manifest struct { ID string `json:"id"` } `json:"manifest"` }

(not necessarily with inline structs, I'm just being lazy here 🙈 )

zsiec added 4 commits June 14, 2019 18:39

provider: adds new bitmovin provider using new sdk

701bf6a

patch: removes go-e files from patching

a993c47

format: goimports

9d4b436

provider: extract method for pulling video configs with custom data

13fd74b

configurations: make one fewer request when deleting a configuration

a0b89f6

zsiec mentioned this pull request Jun 16, 2019

hybrik: fix bug with improper connections #211

Merged

jonathanhperry approved these changes Jun 17, 2019

View reviewed changes

jamescyeh reviewed Jun 18, 2019

View reviewed changes

fsouza reviewed Jun 19, 2019

View reviewed changes

cr: address formatting comments

0e26b5e

fsouza reviewed Jun 24, 2019

View reviewed changes

zsiec force-pushed the new-bitmovin-provider branch from dabe57d to 0e26b5e Compare June 26, 2019 14:52

zsiec closed this Sep 15, 2019

zsiec deleted the new-bitmovin-provider branch September 15, 2019 17:12

	type CustomData *map[string]map[string]interface{}
	type CustomData map[string]map[string]interface{}

proposal: add new refactored bitmovin provider using new sdk #210

proposal: add new refactored bitmovin provider using new sdk #210

Uh oh!

Conversation

zsiec commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zsiec commented Jun 15, 2019

Uh oh!

jonathanhperry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsiec Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsouza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fsouza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zsiec commented Jun 14, 2019 •

edited

Loading

codecov-io commented Jun 15, 2019 •

edited

Loading

zsiec Jun 17, 2019 •

edited

Loading