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

Removed chunk option. Made live mode use maximum payload size #862

Closed
wants to merge 2 commits into from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Sep 6, 2019

Fixes #702

@ethouris ethouris added this to the v1.4.0 milestone Sep 6, 2019
@ethouris ethouris added [apps] Area: Test applications related improvements Type: Maintenance Work required to maintain or clean up the code Priority: High Status: Review Needed labels Sep 6, 2019
@maxsharabayko
Copy link
Collaborator

Use cases

  • With stdin as an input, srt-live-transmit should read by chunks equal to 1316 bytes, as recommended for live mode. But a user should be able to configure it.
  • When FEC is enabled, payload size of packets can't exceed maximum payload - 4 bytes for FEC header. What happens when maximum payload size is set to 1456 (error or ignored)?
  • Check streaming of packets with payload size less than 1316 bytes.

@ethouris
Copy link
Collaborator Author

ethouris commented Sep 10, 2019

Use cases

  • With stdin as an input, srt-live-transmit should read by chunks equal to 1316 bytes, as recommended for live mode. But a user should be able to configure it.

No, now the app is trying to read the maximum size of a single payload allowed in live mode.

It doesn't hurt reading from console because even though you don't have boundaries when reading from console, you will have available number of bytes at once always the size of a single live payload (AFAIR this is what happens when you use the input from ffmpeg -re). So, in normal situation you should read at most 1316 bytes from the console, even if the buffer size is 1456.

When reading from UDP, you either have appropriate size (at most 1456) of a single UDP packet, or it will simply read up to the buffer size (and drop a part of it). If you read less, it doesn't hurt. And more you cannot read. So it is useless to give a user possibility to change this size.

  • When FEC is enabled, payload size of packets can't exceed maximum payload - 4 bytes for FEC header. What happens when maximum payload size is set to 1456 (error or ignored)?

Fixed. The application sets 1456 maximum size first, but then the packetfilter option when configured with builtin fec will make the size fixed to 1452. Reporting error is a wrong idea because in case when you have the filter not configured on agent and configured on peer, it will have to be fixed at the moment when the handshake is made (agent has no idea that 1456 will be too big payload size until it gets handshake response).

In case when you first configure the filter and try to change the SRTO_PAYLOADSIZE option to a value larger than possible with FEC filter, you'll get an error, of course.

  • Check streaming of packets with payload size less than 1316 bytes.

Happens with every test. Streams from Makito show often packets less than 1316 bytes. And for UDP it wouldn't matter anyway, I'd predict possibly some accidental packet merging when reading from stdin, but not that it would be introduced by this change.

@@ -720,9 +713,9 @@ int main(int argc, char** argv)
while (dataqueue.size() < 10)
{
std::shared_ptr<bytevector> pdata(
new bytevector(cfg.chunk_size));
new bytevector(transmit_chunk_size));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the payload size of the target can be used if it is SRT and if source is STDIN. This way stdin->SRT&payload_size=1316 will be supported.
This can be determined on line 664 above, when case SRTS_CONNECTED:.
Then maybe we should not read from source until we have a target connected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If really needed for the console input, it can be also enforced by adding an attribute for the console spec URI: file://con?pkt_size=1316 (following the parameter name used by ffmpeg for UDP). This will enforce the console to read the packet up to this size.

The payload size as set to maximum in SRT will suffice anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is actually a good alternative.
file://con?pkt_size=1316

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the Source should have a packet size option instead 🤔 So you don't rely on the global transmit_chunk_size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The application doesn't predict to have any source or target related options - other than those handled in URI. This one is actually an option specific for the console. In UDP, for example, it might be desired that this option be handled, but different way - when a single UDP reading provides more data, the application should break the transmission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements Priority: High Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

srt-live-transmit default chunk size
2 participants