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

Fallback codecs #108

Closed
wants to merge 2 commits into from
Closed

Fallback codecs #108

wants to merge 2 commits into from

Conversation

at-wat
Copy link
Member

@at-wat at-wat commented Mar 12, 2020

Add a wrapper function to initialize codec and try next fallback codec if one fails.

Reference issue

Fixes #107

Add a wrapper function to initialize codec and try next fallback
codec if one fails.
@at-wat at-wat marked this pull request as ready for review March 13, 2020 12:52
@at-wat at-wat requested a review from lherman-cs as a code owner March 13, 2020 12:52
@lherman-cs
Copy link
Member

I feel like we need to redesign the codec package. This change definitely works. But, it feels that there's a disconnection between the library and the users.

  1. We suddenly require them to register the codecs manually when they want to have a fallback
codec.Register(webrtc.VP8, codec.VideoEncoderFallbacks(
		// Use if hardware acceleration is available.
		codec.NamedVideoCodec{
			Name:  "vaapi",
			Codec: codec.VideoEncoderBuilder(vaapi.NewVP8Encoder),
		},
		// Software implementation should always available.
		codec.NamedVideoCodec{
			Name:  "vpx",
			Codec: codec.VideoEncoderBuilder(vpx.NewVP8Encoder),
		},
	))
  1. Then, when they want to set the codec specific params, we require them to use the names from 1 (this also feels redundant).
		c.CodecParams = map[string]interface{}{
				"vaapi": vpxParam,
				"vpx":   vpxParam,
			}
  1. Also, it's been really weird to me that when they import one of the codec packages, it suddenly registers with one or more payload types:
const (
	PCMU = "PCMU"
	PCMA = "PCMA"
	G722 = "G722"
	Opus = "OPUS"
	VP8  = "VP8"
	VP9  = "VP9"
	H264 = "H264"
)

I think we should instead let the users build the codecs by themselves, and we'll simply take them in GetUserMedia. What do you think?

@at-wat
Copy link
Member Author

at-wat commented Mar 14, 2020

I think we should instead let the users build the codecs by themselves, and we'll simply take them in GetUserMedia.

I'm not very sure what you suggested.
Do you mean removing codec.Register() from init() of each codec package and register required codecs by the user code?

@lherman-cs
Copy link
Member

Sorry, it was definitely very vague. But, yes, you're right. I was thinking that we should remove codec.Register() from init and let the user to explicitly specify the codec in the GetUserMedia.

I think it's really hard to get my idea across with just plain English ^^. So, I've created an experiment here, f6905fe. It's definitely not a working example since I didn't implement everything, but I made changes that I think it's enough to express my idea.

Following are the main changes:

  1. We no longer require the user to specify the Codec Name, pass specific codec params, and import the codec to implicitly register the codec separately anymore. But, we rather have everything in <specific codec>.Params, e.g. x264.Params.
  2. Specifying specific codec params becomes really easy and more straightforward since you just need to call x264.NewParams() and that's the only way you can specify the codec in GetUserMedia, so it's less error-prone. Also, we can avoid an empty interface (which is pretty awesome!)
  3. Since now you're passing the builders, we can have it as an array and multiple builders with the same codec name will be fine. Essentially, we'll get a fallback method automatically.

@at-wat
Copy link
Member Author

at-wat commented Mar 15, 2020

OK. f6905fe definitely makes sense to me.

@at-wat at-wat closed this Mar 15, 2020
This was referenced Mar 17, 2020
@at-wat at-wat deleted the fallback-codec branch March 29, 2020 04:08
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.

Fallback codec
2 participants