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

Fixes to recording #1808

Merged
merged 9 commits into from
Mar 29, 2022
Merged

Fixes to recording #1808

merged 9 commits into from
Mar 29, 2022

Conversation

Arri98
Copy link
Contributor

@Arri98 Arri98 commented Mar 11, 2022

Description

Fixed some issues with recording:

  • Licode would not record streams that didn't have both video and audio. This was fixed by @zafergurel in Fix for recording audio and screensharing streams #1265. Made minor changes for the linter so all credits to him.
  • When stopping the recording, a wrong ID was passed resulting in the stream continued being recorded. This was fixed by @nvazquezg in Fix wrong parameter when stopping recorder #1565
  • When checking if the stream had audio the condition if (getAudioSinkSSRC() == 0) was always returning false even if the stream had audio. In this if block the codec was set to PCMU so if other codec was used the resulting audio is corrupted.

Also removed the toggleRecording button in the basic example and changed it for individual buttons so the user can select the stream to record.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs

No changes

[] It includes documentation for these changes in /doc.
No changes

@Arri98 Arri98 marked this pull request as ready for review March 11, 2022 12:13
Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think this looks good. I do have a question that I feel would avoid adding new methods to set the hasVideo and hasAudio flags when we already have that info when the object is created

log.info(`message: Adding ExternalOutput, id: ${eoId}, url: ${url},`,
logger.objectToLog(this.options), logger.objectToLog(this.options.metadata));
const externalOutput = new erizo.ExternalOutput(this.threadPool, url,
Helpers.getMediaConfiguration(options.mediaConfiguration));
externalOutput.id = eoId;
externalOutput.setHasAudioAndVideo(hasAudio, hasVideo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just pass hasAudio and hasVideo in the constructor just like we do for MediaStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I will update it

carlos and others added 2 commits March 21, 2022 14:50
Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 but please fix the log level as indicated in the comment
Thanks!

@@ -34,7 +34,7 @@ log4j.logger.dtls.DtlsSocketContext=WARN
log4j.logger.dtls.SSL=WARN

log4j.logger.media.ExternalInput=WARN
log4j.logger.media.ExternalOutput=WARN
log4j.logger.media.ExternalOutput=DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider making this WARN again

@lodoyun lodoyun merged commit 4f8e19e into lynckia:master Mar 29, 2022
@zafergurel
Copy link
Contributor

Thanks @Arri98 for giving credit and for your contribution.

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.

3 participants