-
Notifications
You must be signed in to change notification settings - Fork 387
feat: av1 #4500
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
base: next
Are you sure you want to change the base?
feat: av1 #4500
Conversation
…ucing media codec helpers
…utions and maxPicSize
…improve resolution handling
packages/@webex/plugin-meetings/src/multistream/codec/constants.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/constants.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/constants.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/mediaCodecHelper.factory.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/mediaRequestManager.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/mediaCodecHelper.factory.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/mediaCodecHelper.av1.ts
Outdated
Show resolved
Hide resolved
| codecInfo: mediaCodecHelper.getCodecInfo({ | ||
| maxFs: this.getEffectiveMaxFs(), | ||
| maxPicSize: this.getEffectiveMaxPicSize(), | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be possible that maxFs and maxPicSize will be passed at the same time, right? Thinking in the way that I feel, here we need to think about more abstract passing data according to what codec we are currently working with.
Something like
codecInfo = helper.getCodecInfo(this.buildCodecConstraints());
private buildCodecConstraints(): CodecConstraints {
const codec = this.options.preferredCodec;
if (codec === 'av1') {
const maxPicSize = this.getEffectiveMaxPicSize();
return maxPicSize ? {maxPicSize} : {};
}
// default h264 or do if for h264
const maxFs = this.getEffectiveMaxFs();
return maxFs ? {maxFs} : {};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is tricky because at one side I want to delegate all codec checks to mediaCodecHelper but at the same time here we need different info for different codecs
Maybe solution would be instead of calling two getEffectiveX() methods to pass references to it?
const codecInfo = mediaCodecHelper.getCodecInfo({
maxFs: () => this.getEffectiveMaxFs(),
maxPicSize: () => this.getEffectiveMaxPicSize(),
})So won't calculate useless stuff but av1/h264 will call method that they want to use?
… in media codec helper
…ling to use string literals
| export type { | ||
| /** @deprecated use CodecInfo from @webex/plugin-meetings/src/codec/types instead */ | ||
| CodecInfo, | ||
| } from './codec/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this types are used by children repositories, we need to have backward compatibility for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the correct way to deprecate things for the SDK. Should we take care to update this or at least create a task for such changes? Could the SDK team address these deprecations?
packages/@webex/plugin-meetings/src/multistream/codec/mediaCodecHelper.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/multistream/codec/mediaCodecHelper.av1.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| '1080p': { | ||
| maxPicSize: 2_359_296, | ||
| levelIdx: 9, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I believe @k-wasniowski said that we won't support the different levelIdx, ot I'm missing something?
|
|
||
| return [ | ||
| WcmeCodecInfo.fromAv1( | ||
| 45, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we have any constant for this param here?
| resolution = PANE_SIZE_TO_RESOLUTION[paneSize]; | ||
| } else { | ||
| LoggerProxy.logger.warn( | ||
| `MediaCodecHelperAV1#getMaxPicSize --> unsupported paneSize: ${paneSize}, using "medium" instead` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can use in log PANE_SIZE_TO_RESOLUTION.medium
| if (mr.codecInfo?.codec !== 'h264') { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if returning 0 silently is good idea here for an incorrect codec. It would be good to log info that unexpected mr.codecInfo?.codec is passed here.
| export type { | ||
| /** @deprecated use CodecInfo from @webex/plugin-meetings/src/codec/types instead */ | ||
| CodecInfo, | ||
| } from './codec/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the correct way to deprecate things for the SDK. Should we take care to update this or at least create a task for such changes? Could the SDK team address these deprecations?
|
|
||
| type ClientRequestsMap = {[key: MediaRequestId]: MediaRequest}; | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this comment for ESLint? It wasn't here before
| getCodecInfo(options: { | ||
| getMaxFs?: () => number; | ||
| getMaxPicSize?: () => number; | ||
| }): CodecInfo | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we need to improve this part here...The shared signature in MediaCodecHelper forces every helper to accept both getMaxFs and getMaxPicSize, even though only one makes sense per codec.
Can we have here generic helper interface for getCodecInfo and export concrete types MediaCodecHelperH264?
something like
interface MediaCodecHelper<TOptions, TCodecInfo> {getCodecInfo(options: TOptions): TCodecInfo | undefined; ... }
| codecInfo: mediaCodecHelper.getCodecInfo({ | ||
| getMaxFs: () => this.getEffectiveMaxFs(), | ||
| getMaxPicSize: () => this.getEffectiveMaxPicSize(), | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have here after generic helper interface for getCodecInfo impromevents something like:
const codecInfo =
mediaCodecHelper.codec === 'h264'
? mediaCodecHelper.getCodecInfo({getMaxFs: () => this.getEffectiveMaxFs()})
: mediaCodecHelper.getCodecInfo({getMaxPicSize: () => this.getEffectiveMaxPicSize()});
COMPLETES SPARK-728169
This pull request addresses
The need to add AV1 codec support to the Webex JS SDK multistream functionality. AV1 (AOMedia Video 1) is a modern, open-source video codec that provides superior compression efficiency compared to H.264, enabling better video quality at lower bitrates. This enhancement allows the SDK to leverage AV1 codec capabilities for improved video streaming performance in multistream scenarios.
by making the following changes
Architecture Improvements
MediaCodecHelper- Abstract base class defining the codec helper interfaceMediaCodecHelperAV1- AV1-specific implementation with maxPicSize and frame size calculationsMediaCodecHelperH264- H.264-specific implementation maintaining existing behaviorMediaCodecHelperFactory- Factory class for creating appropriate codec helpersCore Functionality
AV1 Codec Support: Added comprehensive AV1 codec parameter handling including:
@webex/internal-media-coreAV1Codec classEnhanced Media Request Manager: Refactored
mediaRequestManager.tsto:Remote Media Enhancements: Updated remote media handling to:
Code Quality
Package Updates
@webex/media-helpersand@webex/plugin-meetingspackage versionsChange Type
The following scenarios were tested
Automated Tests
Manual Testing
Integration Scenarios
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Summary Statistics
Make sure to have followed the contributing guidelines before submitting.