Add frameDuration attribute to OpusEncoderConfig #551

bdrtc · 2022-09-05T06:29:23Z

This fixes #371

dalecurtis · 2022-09-06T17:02:19Z

opus_codec_registration.src.html

@@ -131,6 +131,7 @@
 <xmp>
 dictionary OpusEncoderConfig {
  OpusBitstreamFormat format = "opus";
+  [EnforceRange] unsigned long frameDuration;


Should probably be "long long" and in microseconds like all other time fields in WebCodecs.

we do find duration in audioData is in microseconds,
as described in #371 ,
frameSize = numberOfChannels*sampleRate * frameDuration / 1000
most audio encoder use millseconds as frame duration(opus ptime for example), we can change it to micoseconds, then ,the frameSize Calculation will be:
frameSize = numberOfChannels*sampleRate * frameDuration / 1000/1000

Thanks.

@dalecurtis Do you see the new commit according to your comments? This is my first PR that need some modifications, I'm afraid of missing an important action.

We can see the new commits. This issue will be discussed at TPAC next week, so we might hold off on merging this PR until those discussions happen.

I defer to Thomas for the final decision, I was just driving by.

@aboba, @padenot: any thoughts on microseconds vs milliseconds?

IIUC, the valid frameDurations in milliseconds are {2.5, 5, 10, 20, 40, 60} according to the spec. Leaving it in microseconds might give the impression of finer grain support.

As you say, Opus ptimes have a limited set of values, are expressed in ms and are rounded up. So if you're willing to make the lowest frameDuration 3 instead of 2.5, you don't really need microseconds.

…ng, and meaning of it to microseconds as well

tguilbert-google · 2022-09-17T00:19:09Z

@aboba, @padenot : does this need editor's approval? If so, please take a look.

padenot · 2022-09-19T23:39:53Z

We probably want to have something to do the error checking/validation/rounding to nearest here. Only a very small set of numbers is valid. What happens if you pass something that Opus can't work with?

bdrtc · 2022-09-20T03:36:42Z

if we want add error checking, it should be run in audioencoder configure method, by checking the validation of AudioEncodeConfig, Add one more check step like this:
2. if codec is opus and frameDuration exist, frameDuration is not in {2.5, 5, 10, 20, 40, 60}, return false.
what's more, frameDuration type should be changed to double.
@tguilbert-google @padenot any more sugguestion about this ? thanks.

aboba · 2022-09-20T18:12:00Z

I think the allowable values are a bit broader. From RFC 7587 Section 6.1:

" ptime: the preferred duration of media represented by a packet
(according to Section 6 of [RFC4566]) that a decoder wants to
receive, in milliseconds rounded up to the next full integer
value. Possible values are 3, 5, 10, 20, 40, 60, or an arbitrary
multiple of an Opus frame size rounded up to the next full integer
value, up to a maximum value of 120, as defined in Section 4. If
no value is specified, the default is 20."

tguilbert-google · 2022-09-20T20:39:15Z

if codec is opus and frameDuration exist, frameDuration is not in {2.5, 5, 10, 20, 40, 60}, return false.

I think we are trying to avoid codec-specific steps in the main spec. We could instead add a generic validation step referring to the registries, along the lines of "If the current codec's registry defines a codec-specific Additional configuration validation steps, run the algorithm defined in the registry. If the return value is ...", and add a section validating OpusEncoderConfig.

There is probably better wording though.

padenot · 2022-09-20T20:45:54Z

I agree with @tguilbert-google.

…g; add validation checking steps to it

bdrtc · 2022-09-21T08:14:15Z

We add a new commit according to the disscussion above, please take a review. @tguilbert-google

tguilbert-google

Latest commit looks good % comments.

FYI, I will be generally unavailable in the next week and a half.

tguilbert-google · 2022-09-23T03:58:31Z

index.src.html

-2. Return `true`.
+2. If the current codec's registry defines a codec-specific additional configuration
+    validation steps, run the algorithm defined in the registry. If the return
+    value is `false`, return it.


How about:

2. If the {{AudioEncoderConfig}} has a codec-specific extension and the corresponding registration in the [[WEBCODECS-CODEC-REGISTRY]] defines steps to check whether the the extension is a valid extension, return the result of running those steps. 3. Return `true`.

tguilbert-google · 2022-09-23T03:58:39Z

opus_codec_registration.src.html

+To check if an {{OpusEncoderConfig}} is a valid OpusEncoderConfig,
+run these steps:
+1. If {{OpusEncoderConfig/frameDuration}} is not a valid ptime value,
+    which is described in section 6.1 of [[RFC6381]], return `false`.


I'm not sure this is the right RFC/section. Should this be Section 6.1 of rfc7587?

Defer to @aboba.

I also don't know if there's a way to directly link to a section from the [[RFC...]]

Yes, It's a wrong reference. It should be rfc7587.

I will fix it.

Yes, RFC 7587 Section 6.1 makes more sense. We typically link to the RFC and include the Section # but not link to it.

tguilbert-google · 2022-09-23T04:07:40Z

opus_codec_registration.src.html

 };
 </xmp>
 </pre>

+To check if an {{OpusEncoderConfig}} is a valid OpusEncoderConfig,


NIT: <dfn>valid OpusEncoderConfig</dfn>

Cause there is not a more detail dfn about "valid OpusEncoderConfig", maybe a clever desc is:
To check if an {{OpusEncoderConfig}} is valid,
......

helaozhao · 2022-09-23T10:58:22Z

another commit, please review it.

tguilbert-google · 2022-09-23T18:35:12Z

Looks good to me!

I'm leaving editors @aboba and @padenot to do the final approval and merge.

Thanks for the patch!

tguilbert-google · 2022-09-23T18:32:00Z

opus_codec_registration.src.html

 };
 </xmp>
 </pre>

+To check if an {{OpusEncoderConfig}} is valid, run these steps:
+1. If {{OpusEncoderConfig/frameDuration}} is not a valid ptime value,


NIT: should ptime be ptime? (wrapped with backticks?)

Yes, it should be.

bdrtc · 2022-09-29T08:26:08Z

@aboba @padenot any thoughts about new commit?

tguilbert-google · 2022-10-06T21:30:50Z

Assuming no objections from @padenot, I am merging this change.

Chromium implementation work will be tracked in crbug.com/1372152

tguilbert-google · 2022-10-06T21:31:03Z

Thanks for the PR!

@tguilbert-google