Fix recording of audio only or video only streams

[nvazquezg]

Description

ExternalOutput was discarding streams that didn't have video and audio.
This commit enables the output for streams that have at least one of them.

This PR should solve issues #1203 and #1185.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs

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

[zafergurel]

This fixes screensharing and audio recording.
But it breaks video recording.
The following error occurs in the logs:

Only audio, video, and subtitles are supported for Matroska

The result recording file includes just audio data when webcam is shared.

[kekkokk]

This is quite ugly

I would rather set global var hasAudio and hasVideo and set the correct index when writing the packet:

licode/erizo/src/erizo/media/ExternalOutput.cpp

Line 207 in 24c955a

av_packet.stream_index = 1;

and

licode/erizo/src/erizo/media/ExternalOutput.cpp

Line 298 in 24c955a

av_packet.stream_index = 0;

[nvazquezg]

Couldn't agree more, I'll try with a global hasAudio and hasVideo. Thanks.

[zafergurel]

Hi @nvazquezg and @kekkokk,
Regarding your comments, I've made a new pull request (#1265).
Could you please review it?
With this commit, licode records audio-only and video-only streams correctly as well as video-audio streams.

[zafergurel]

When we use init_video to set video stream to null, we don't have a proper recorded video.
The init_context is called more than once. If video_stream_ has been initialized before some audio data are being written, init_video stays as false and context->streams[0] is set to null stream which garbles the recording file.

As far as I understand, if video_codec_ is null, it means we won't receive video data.
Also same for audio. if audio_codec_ is null, there will be no audio data.

So instead of using init_video to check if this is just an audio stream, we can check video_codec_:

`

//TODO: this 'if' is to avoid a matroska error when no video available: 'Invalid packet stream index: 1'
if (video_codec_ == AV_CODEC_ID_NONE) {
    // To avoid the following matroska errors, we add CODEC_FLAG_GLOBAL_HEADER...
    // - Codec for stream 0 does not use global headers but container format requires global headers
    // - Only audio, video, and subtitles are supported for Matroska.
    video_stream_ = avformat_new_stream(context_, nullptr);
    video_stream_->codec->flags |= CODEC_FLAG_GLOBAL_HEADER;
    context_->streams[0] = video_stream_;
}

`
That fixed my problem. Now, video is also being recorded properly.
Also, setting the global header is required to avoid some matrosko errors.

[zafergurel]

After some tests, I've realized that recorded video is shorter than expected.
Using the master branch version (without this fix), the video is 5 seconds shorter (for a 15 second recording). I think that's because the remaining video frames in the queue cannot be recorded properly.

With this fix, 5 second part is missing at the start as well. I think that's because the audio data come after and re-writes the file. That's a guess, I'm not sure.

I couldn't get the recording feature work in a stable mode.

I think some work needs to be done about recording feature.

[nvazquezg]

Thanks for the feedback, I'll try to improve this behaviour and come back.

[zevarito]

@nvazquezg @zafergurel Could you check this PR[0] when you have chance?
It is a replacement of libav with ffmpeg but has a sustancial rework around ExternalOutput, it might solve some of the issues you are mention or at least it is probably much easier to take care of them now.

[0] #1218

[nvazquezg]

@zevarito I've tested it a couple of weeks ago and I found no problems. But the issue I'm trying to fix here is related to the recording of outputs with only audio or video stream and this was not working in your PR.
The main reason is this if that discards outputs that don't have both streams:

https://github.com/zevarito/licode/blob/8dcf31b2ba5a004cc45890b662e7baa781ecb54f/erizo/src/erizo/media/ExternalOutput.cpp#L420-L422

I'm just starting to understand the inner workings of ExternalOutput, but I'll check if I find an easier solution to this problem in your PR.

[zevarito]

@nvazquez cool let me know if you have questions about it. That PR only needs manual testing to ensure no regressions and stability, specially on recording. I am kinda far of the project right now so I couldn’t test deeper myself.

On Thu, Jul 12, 2018 at 11:23 nvazquezg ***@***.***> wrote: @zevarito <https://github.com/zevarito> I've tested it a couple of weeks ago and I found no problems. But the issue I'm trying to fix here is related to the recording of outputs with only audio or video stream and this was not working in your PR. The main reason is this if that discards outputs that don't have both streams: https://github.com/zevarito/licode/blob/8dcf31b2ba5a004cc45890b662e7baa781ecb54f/erizo/src/erizo/media/ExternalOutput.cpp#L420-L422 I'm just starting to understand the inner workings of ExternalOutput, but I'll check if I find an easier solution to this problem in your PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1254 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACgamuYr5PVb6PI3oL-ubcY7veyO1-Oks5uF1vNgaJpZM4VBZ50> .

-- Sent from Gmail Mobile

[nvazquezg]

This PR #1265 offers a better solution to this problem than this one.