-
Notifications
You must be signed in to change notification settings - Fork 332
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
feat(metrics): Add client.media.render.start event #3555
Conversation
@@ -7702,6 +7702,18 @@ export default class Meeting extends StatelessWebexPlugin { | |||
) { | |||
layoutInfo.content = {width: contentWidth, height: contentHeight}; | |||
} | |||
|
|||
// @ts-ignore | |||
this.webex.internal.newMetrics.submitClientEvent({ |
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.
This place here is only applicable to transcoded meetings. For multistream meetings, the layout is changed via RemoteMediaManager.setLayout() method. Also it doesn't feel right to be sending "client.media.render.start" at the point when we switch layout. Probably at this point we should be sending "client.share.layout.displayed" and "client.media.render.start" should I guess be sent when first frame is decoded and displayed. This will be tricky for SDK to know, because it doesn't have a reference to the video element that renders the stream and it only polls getStats() API every 5s so it can't immediately detect when frames are decoded - it may detect it up to 5s later. So maybe as a compromise we could send it when the share ReceiveSlot's sourceState changes to "live", but it still poses some challenges:
- in theory there can be many share receive slots, which one should send it? the first one or all of them?
- ReceiveSlots are not used for transcoded meetings/calls
@@ -8630,34 +8652,6 @@ describe('plugin-meetings', () => { | |||
|
|||
checkParseMeetingInfo(expectedInfoToParse); | |||
}); | |||
|
|||
it('should parse meeting info, set values, and return null when permissionToken is not present', () => { |
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 is this unrelated test being removed as part of this PR? is this some merge gone wrong?
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 didn't remove this? I'll see what happened here
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.
Ok I must've command + z'd this test out of the file. Good catch!
COMPLETES #<https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-518038>
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios where tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.