Replay data channel recordings

[ricardo-salgado-tekever]

Added capability to replay data recordings in recordplay plugin from streaming plugin.

[januscla]

Thanks for your contribution, @ricardo-salgado-tekever! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

Thanks! But I noticed several issues. You can find my comments inline.

[lminiero]

Thanks for the quick fixes! Added a few other notes.

That said, I think this still needs work: I don't see any change to how janus_recordplay_get_frames processes frames, and I see the same method is used as it is for data channels. This means that the code will try to reorder frames working with a non-existing RTP header, which will not work, and will not time the messages correctly. Please see how janus-pp-rec.c traverses data recordings in a different way (it should be easy enough to port as it's a separate check), by skipping to the right offset and most importantly using the time information in the mjr to time the messages properly.

[ricardo-salgado-tekever]

Interestingly, even without changes to janus_recordplay_get_frames, this does actually work. And the timing comes out OK, as odd as it may sound.
What is happening is that the non-existent RTP header fields end up aligning the timestamp field with the least significant word of the timestamp :-D - and it works!
Nonetheless, this is not a proper solution, so I will port from janus-pp-rec.c.

One more point, it looks like there is a bug there reading the timestamp: this bytes = fread(&when, sizeof(gint64), 1, file); should return 1 instead of bytes.
Per documentation:

Return Value
The total number of elements successfully read are returned as a size_t object, which is an integral data type. If this number differs from the nmemb parameter, then either an error had occurred or the End Of File was reached.

[ricardo-salgado-tekever]

Now with a proper implementation on janus_recordplay_get_frames for data recordings, based no janus-pp-rec.c.
Note that janus-pp-rec.c has a bug in reading data packets header, I will submit a correction in another pull request.

[ricardo-salgado-tekever]

Once this one is merged - but not before we dot all the i's and cross all the t's - I have seek capability already working on the playout. It builds on this branch.

[lminiero]

I've added a small note inline: including the missing swap of sizeof(gint64), 1 addressed in the previous note, I think those are the last parts that need to be fixed before this is ready.

I'll make a couple of tests myself in the meanwhile to see if I spot anything else. Thanks!

[lminiero]

If you play a recording with data, and then play a recording without data, Janus crashes here:

==24334==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f5e6d8d1631 bp 0x000000000000 sp 0x7f5e63707428 T14)
==24334==The signal is caused by a READ memory access.
==24334==Hint: address points to the zero page.
    #0 0x7f5e6d8d1631 in __strstr_sse2_unaligned (/lib64/libc.so.6+0xa9631)
    #1 0x7f5e6e6fca30 in strstr (/lib64/libasan.so.6+0x8ba30)
    #2 0x7f5e6969271e in janus_recordplay_playout_thread plugins/janus_recordplay.c:2536

which is

	if(session->dframes) {
		char source[1024];
		if(strstr(rec->drc_file, ".mjr"))            <----

This is very likely because session->dframes is not cleared up when the playout ends, and so it's still there for the following playout that doesn't have any data recording. Since rec->drc_file is NULL, the method crashes.

The cleanup is missing at the end of janus_recordplay_playout_thread:

	audio = session->aframes;
	while(audio) {
		tmp = audio->next;
		g_free(audio);
		audio = tmp;
	}
	session->aframes = NULL;
	video = session->vframes;
	while(video) {
		tmp = video->next;
		g_free(video);
		video = tmp;
	}
	session->vframes = NULL;

This should now include the same for dframes.

[lminiero]

One more note: you're never freeing recording->drc_file, so you're also introducing a memory leak. It should be done in janus_recordplay_recording_free, which is where arc_file and vrc_file are freed too.

[ricardo-salgado-tekever]

Yeah, too much time spent on garbage-collected environments. Need to clean-up after myself.

[lminiero]

Yeah, too much time spent on garbage-collected environments. Need to clean-up after myself.

C is ruthless!! 🤭

[lminiero]

Thanks for the quick fixes! I don't see anything else that needs to be addressed, so this can be merged 👍