Add support of abs-capture-time extension to streaming plugin and forwarding of it's value from rtp source

[IbrayevRamil]

1. Added support of abs-capture-time extension in streaming plugin
2. Added parsing of abs-capture-time extension from RTP source for forwarding it further to webrtc
3. Removed memset(index+9, 0, 8); as it doesn't make sense to fill rest of bytes with zeroes if we parse only 8 bytes of extension.

[lminiero]

I'm not sure this works, at least not without guidance. The code to parse the extension from incoming packets is the following:

	if(janus_rtp_header_extension_parse_abs_capture_time((char *)packet->data, packet->length,
				janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME), &abs_ts) == 0) {
		rtp.extensions.abs_capture_ts = abs_ts;
	}

This means that the plugin is expecting the extension to have a very specific ID, the one returned by janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME) (which in your patch is 7). There's no guarantee that will be the case, and nowhere does it say that should be it. Sources may use a different one, meaning that the code might either not find it (wrong ID), or even worse parse something that is something else entirely (extension ID is used for a different extension).

I guess one way to address it might be to change the configuration of the property: instead of a boolean, an integer that specifies the ID to look for. This would allow people in control of the media source to tell the mountpoint exactly which ID to look for, to extract that specific information, making it more flexible and configurable.

[IbrayevRamil]

I'm not sure this works, at least not without guidance. The code to parse the extension from incoming packets is the following:

	if(janus_rtp_header_extension_parse_abs_capture_time((char *)packet->data, packet->length,
				janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME), &abs_ts) == 0) {
		rtp.extensions.abs_capture_ts = abs_ts;
	}

This means that the plugin is expecting the extension to have a very specific ID, the one returned by janus_rtp_extension_id(JANUS_RTP_EXTMAP_ABS_CAPTURE_TIME) (which in your patch is 7). There's no guarantee that will be the case, and nowhere does it say that should be it. Sources may use a different one, meaning that the code might either not find it (wrong ID), or even worse parse something that is something else entirely (extension ID is used for a different extension).

I guess one way to address it might be to change the configuration of the property: instead of a boolean, an integer that specifies the ID to look for. This would allow people in control of the media source to tell the mountpoint exactly which ID to look for, to extract that specific information, making it more flexible and configurable.

But you apply the same logic for JANUS_RTP_EXTMAP_ABS_SEND_TIME and rest of extensions, so why exactly this one should have another logic?
I’m not sure that it makes sense as Janus doesn’t simply forwards all of the rtp extension headers and this extension is disabled by default. Probably, it makes sense to add information about id to the description of boolean flag.

[lminiero]

But you apply the same logic for JANUS_RTP_EXTMAP_ABS_SEND_TIME and rest of extensions, so why exactly this one should have another logic?

Because those are extensions where we generate a value ourselves. Your extension is the first one where the data actually comes from somewhere else, and so we need to find/parse it first.

[IbrayevRamil]

But you apply the same logic for JANUS_RTP_EXTMAP_ABS_SEND_TIME and rest of extensions, so why exactly this one should have another logic?

Because those are extensions where we generate a value ourselves. Your extension is the first one where the data actually comes from somewhere else, and so we need to find/parse it first.

Ok, I agree with you. I will try to make it numeric with 0 by default which means no extension.

[lminiero]

Ok, I agree with you. I will try to make it numeric with 0 by default which means no extension.

It could also be a separate property: meaning, there still is a boolean as now (that offers the extension in the SDP as you do now, with the hardcoded value), and a different property to tell the code which extension ID is used in the source, so for parsing. I think this would actually be a good idea, since the Streaming plugin now has the option to receive offers from viewers too, which means the extension ID to use may be set by the client; besides, we'd want the extension ID not to conflict with other ones we use. In both cases, to avoid conflicts it's a good idea to let the plugin decide what ID to use on the WebRTC side, which doesn't need to be the same as the one the RTP source uses.

[IbrayevRamil]

Ok, I agree with you. I will try to make it numeric with 0 by default which means no extension.

It could also be a separate property: meaning, there still is a boolean as now (that offers the extension in the SDP as you do now, with the hardcoded value), and a different property to tell the code which extension ID is used in the source, so for parsing. I think this would actually be a good idea, since the Streaming plugin now has the option to receive offers from viewers too, which means the extension ID to use may be set by the client; besides, we'd want the extension ID not to conflict with other ones we use. In both cases, to avoid conflicts it's a good idea to let the plugin decide what ID to use on the WebRTC side, which doesn't need to be the same as the one the RTP source uses.

Yes, did exactly the same. You can review now

[lminiero]

Added a few notes inline. Thanks!

[IbrayevRamil]

@lminiero kind reminder

[lminiero]

It looks fine to me, now, even though I have no way to test it, since I don't have sources that generate such an extension. That said, I don't like the name of the config property much: too long and too verbose. Maybe abscapturetime_ext_id instead of abscapturetime_src_ext_id? I guess you added the src to highlight it's the ID of the source, and not what is advertised in the SDP on the WebRTC side, but people are supposed to read the documentation before using things, and the comment does explain this.

That said, this is a very minor note and probably not even that important (cutting 4 characters from the name doesn't really make it less verbose anyway).

[IbrayevRamil]

It looks fine to me, now, even though I have no way to test it, since I don't have sources that generate such an extension. That said, I don't like the name of the config property much: too long and too verbose. Maybe abscapturetime_ext_id instead of abscapturetime_src_ext_id? I guess you added the src to highlight it's the ID of the source, and not what is advertised in the SDP on the WebRTC side, but people are supposed to read the documentation before using things, and the comment does explain this.

That said, this is a very minor note and probably not even that important (cutting 4 characters from the name doesn't really make it less verbose anyway).

Hi, I've created docker image, which generates test RTP video stream with abs_capture_time inside (id=7) and streams it to the specified host and port, so you could test it locally. I'm using gstreamer and its videotestsrc plugin.

It's assumed that you have created mount point with following config:

test: {
	type = "rtp"
	id = 10
	description = "Test abs_capture_time"
	abscapturetime_src_ext_id = 7
	media = (
		{
			type = "video"
			mid = "test-abs"
			port = 8100
			pt = 126
			codec = "h264"
		}
	)
}

Then you should run docker container with the following command:
docker run --rm ramilkz/abs_captutre_time_test:latest {janus_host} {mountpoint_port}

If you are running janus locally and using MacOS then:
docker run --rm ramilkz/abs_captutre_time_test:latest host.docker.internal 8100

If you are running janus locally and using Linux then:
docker run --rm --add-host=host.docker.internal:host-gateway ramilkz/abs_captutre_time_test:latest host.docker.internal 8100
or just use host mode
docker run --rm --network=host ramilkz/abs_captutre_time_test:latest localhost 8100

Then you can check in Wireshark or any other tool that extension exists alongside other janus headers in packets.

!!! I put extension header only into marker packets to avoid overhead, so it's okay that it doesn't exist in non-marker packets

[lminiero]

Apologies for these delays in responses. I've just identified a couple of very minor things to change, and after that it's good to merge for me 👍

[IbrayevRamil]

@lminiero rebased onto current master and also squashed commits

[lminiero]

Thanks, merging!