ultravideo / uvgrtp Goto Github PK

If I pull h.264 frames directly from rtsp in a live stream, I can feed it through ffmpeg:
ffmpeg -i rtsp://192.168.1.5/0/profile2/media.smp out.h264

I can't understand how to do the same using this library. I'm able to go through the rtsp setup using libcurl, then capture bits with uvgrtp, but after I write them to file, they are unplayable by ffmpeg.

    uvgrtp::context ctx;
    uvgrtp::session *sess = ctx.create_session("127.0.0.1");
    uvgrtp::media_stream *vid = sess->create_stream(1234, 0, RTP_FORMAT_H264, RTP_NO_FLAGS);
    vid->install_receive_hook(nullptr, receive_hook);
...
void receive_hook(void *arg, uvgrtp::frame::rtp_frame *frame) {
    fout.write((const char*)frame->payload, frame->payload_len);
}

It appears as though they are no longer wrapped with NAL. The bits on my ffmpeg-grabbed file have the 4 byte 0x1 prefix, but when I'm writing via uvgrtp, they don't, though the next 4 bytes show that they clearly share some of the actual contents (and appear roughly the same size).

ffmeg hexdump -C output starts:
00000000  00 00 00 01 67 64 00 28

receive_hook hexdump -C output starts:
00000000  67 64 00 28 ac 1b 1a 80

Also, when I print out the frame sizes, they seem to contain frames that approximate inter- and intra- sizes, but also some other meta-type frames (usually 8 bytes):

len: 48
[RTPLIB][DEBUG][::call_aux_handlers] Received a corrupted packet!
len: 4
len: 26
len: 58134
len: 8
len: 3236
len: 8
len: 3109
len: 8
len: 3582
len: 8

What am I missing? I've looked for examples but came up short. I tried RTP_FORMAT_GENERIC and seemed to have similar results, and same with h.265.

Hello again,
I am trying to send APP packets for testing purposes. The output is the following:

I dug around and the problem is that you define frame types in the enum like so:

But the handler in the rtcp::handle_incoming_packet sets the max on BYE packet, whereas in reality max is the APP packet.

I don't know if I should create pull requests for such minor fixes, please let me know.

Thank you

I missed some of the duplicate code in formats packet handlers the first time I was eliminating duplicate code. These handlers should be reformatted to remove duplicate code.

Some codecs use predefined payload types, but many codecs use a dynamic payload type. Currently there is no way to set the dynamic payload number for Opus, VVC, HEVC or AVC, when all of these use a dynamic number. Instead the library forces the number to be some predetermined value.

See more details here

I would like to at some point change the internal architecture to have a more structured approach. A figure of the proposed architecture:

Currently many of the components implement their own flow. In my opinion it would be better if there was one class that would organize the processing flow between different components, especially since many components have several clearly defined stages. I would use the existing packet dispatcher as the new controller.

I have not yet started working on this.

Hello,

there is a problem with the names of the APP packets. API guide states the following:

When in fact, any name that is longer than 3 characters results in name corruption. For example, a packet named "123" produces:

whereas packet named "1234" produces:

In the rtcp_app_packet declaration the name is represented as

So it is converted to an array with a length of [4] and is probably null-terminated, because the function takes char pointer

 rtp_error_t send_app_packet(char *name, uint8_t subtype, size_t payload_len, uint8_t *payload);

So I guess this is the intended behaviour with a misleading API guide. But I think that 3 characters long names are a bit short.
Either the API should be fixed, or the names allowed to be longer, API fix is easier tho :)

p.s also when converting the name from the app packet that we receive back to char*, it becomes non null-terminated.

I am trying to use examples in the document for sending and receiving (pooling).
when I set
#define PAYLOAD_MAXLEN 1000
and got a correct receiving

value: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 
value: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 
value: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 
value: 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 
value: 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 
value: 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 
value: 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 
value: 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 
value: 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 
value: 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 

For data length bigger than 10000, by setting
#define PAYLOAD_MAXLEN 10000
I do not receive data correctly.

value: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 
value: 0, 1, 3, 4, 5, 6, 7, 8, 9, 10, 
value: 2, 1, 4, 5, 6, 7, 8, 9, 10, 11, 
value: 2, 1, 5, 6, 7, 8, 9, 10, 11, 12, 
value: 4, 1, 6, 7, 8, 9, 10, 11, 12, 13, 
value: 4, 1, 7, 8, 9, 10, 11, 12, 13, 14, 
value: 6, 1, 8, 9, 10, 11, 12, 13, 14, 15, 
value: 6, 1, 9, 10, 11, 12, 13, 14, 15, 16, 
value: 8, 1, 10, 11, 12, 13, 14, 15, 16, 17, 
value: 8, 1, 11, 12, 13, 14, 15, 16, 17, 18,

here is my code for sending:

for (int i = 0; i < 10; ++i) {
      for (auto j=0; j < 10; ++j) buffer[j] = (uint8_t)i+j;
      if (hevc->push_frame(buffer, PAYLOAD_MAXLEN, RTP_NO_FLAGS) != RTP_OK)
        fprintf(stderr, "Failed to send RTP frame!");
      std::this_thread::sleep_for(std::chrono::milliseconds(50));
    }

and receiving:

while ((frame = hevc->pull_frame()) != nullptr) 
    {
        if (frame->payload_len)
        {
            printf("value: ");
            for (auto i=0; i < 10; ++i) printf("%d, ", frame->payload[i]);
            printf("\n");
        }
        /* When we receive a frame, the ownership of the frame belongs to us and
         * when we're done with it, we need to deallocate the frame */
        (void)uvg_rtp::frame::dealloc_frame(frame);
    }

I really do NOT understand why RTP causes problem by sending big data like video??
Could you please help me?

I have identified duplicate code in uvgRTP and I'm currently in the process of removing it. There is duplicate code at least in sending RTCP and ZRTP messages and I'm currently creating a helper function that does the same thing for all message types.

Just wanted to let everyone who is working on uvgRTP know this so we don't start doing the same thing.

Currently, uvgRTP public API is very much C-like. It would be highly beneficial to add separate C++ API alongside the C API so smart pointers can be used.

This would not break the existing API. The C API will be supported at least until 3.0 release of uvgRTP and possibly long after it.

Task lists:

• Creation of session with smart pointers
• Creation of media_streams with smart pointers
• Support bind function with RTP receive hook
• Updating the examples to use this new API

Hi all. I'm currently using uvgRTP in my research project.

Since my project has many dependencies, I manage them in the CMake script with pkg-config. As you may know, pkg-config will find a library by its name and add all the flags for the linker and include path to the compile commands. To make that work, pkg-config requires .pc file in PKG_CONFIG_PATH such as /usr/local/lib/pkgconfig.

Till now, I'm using my forked repo and wrote the CMake scripts in that repo. It gets a .in configure file, generate a .pc file, and copy it to the proper location. If you are fine, I'd like to create a PR for this issue.

Thanks,
Jin

Is it possible to use the media streams created from the same session in different threads? They seem to at least share the zrtp between themselves.

This should be checked and an explicit guarantee should be given in documentation.

I don't think it is a good idea to do all the functionality of a zrtp message in its constructor. The operations should be separated to at least a separate function that is called. This change is not trivial since the messages are already using this logic.

This affects all ZRTP message types.

My operating system is Win10, and I have successfully compiled the kvzrtp and generate a static file libkvzrtp.a. But when I try to test the lib on example/simple/sending.cc, I'm confused of how to use it.
I use Visual Studio 2019 and added kvzrtp/src as include path, qtcomplied/debug as library path, but When I complile the sending.cc, I got lots of errors. It's confusing that you offer linking method on Windows is -L -lkvzrtp -lpthread... It seems like the command is linking dll.

Would it be possible to get support for RFC 4175 aka SMPTE 2110 as a feature of this library?

The source seems really "C-ish".
Is this because of some performance concerns or would a PR in this regard be welcome ?
For example, using smart pointer in some situations ?

Thanks for shairing your great work,

I just give it a try and Example build faild here is error as below.

g++ sending.cc -luvgrtp -lpthread

media_stream.cc:(.text+0x6df): undefined reference to uvgrtp::formats::h266::h266(uvgrtp::socket*, uvgrtp::rtp*, int)' media_stream.cc:(.text+0x6e6): undefined reference to typeinfo for uvgrtp::formats::h266'
media_stream.cc:(.text+0x70d): undefined reference to uvgrtp::formats::h266::get_h266_frame_info()' media_stream.cc:(.text+0x71a): undefined reference to uvgrtp::formats::h266::packet_handler(void*, int, uvgrtp::frame::rtp_frame**)'
collect2: error: ld returned 1 exit

Thanks

This issue is a compilation of all improvements suggested for current CMakeLists.txt file. Having all the issues collected under one issue makes managing changes easier and issue list of uvgRTP clearer.

Existing unresolved issues:

• #12 Creation of subfolder uvgrtp in the include folder
• #13 Not using absolute install destination paths
• #37 List header files
• #38 Adding PRIVATE to target_include_directories

Additional suggestions:

• Test integration
• Static code analysis
• Better install integration

@db-tech has expressed interest in doing a first draft of the file. This would be greatly appreciated. I'm not currently working on any of there, but if there is anything I can do to help, feel free to ask me. I'm only starting to learn CMake.

Also if someone has further improvement ideas, mention them in the comments and I can add them this issue. These improvements can be made in small patches or in one big file change. Whatever suits best. Additional improvements in file are also welcome as long as they don't unnecessarily complicate the usage of uvgRTP or CMakeLists file.

I have encountered enough issues with RTCP that the whole RTCP implementation should be checked and compared to RFC 3550.

The issue is related to

• #35
• #30
• #22
• #59
• #91

Few things I would check carefully:

• whether the RTP and RTCP timestamps can be used to synchronize multiple streams based on timestamps from initial source of media.
• Are the field values for rtcp reports calculated correctly such that it is possible to do congestion control.

Just curious. This can occur with really long streams and should work.

Not listing them does not prevent compilation, but it is annoying that they don't show up in the editor.

Hello,
While working with uvgRTP, I believe I've come across a couple of bugs:

When using RTP_FORMAT_GENERIC with the RCE_FRAGMENT_GENERIC flag enabled, individual packets smaller than payload_size are not recognized as such by the receiver and are discarded. I believe this is due to the fact that the sender does put a mark on every packet smaller than the payload_size, regardless if it is a singleton or not.

When using RTP_FORMAT_H264:

• clear_aggregation_info() is not always called before flushing the queue, which can result in a duplicate of the data being inserted into the next message.
• wsa_bufs (socket.cc line 375) can contain only 16 buffers, but there's nothing to prevent inserting more if there are a lot of aggregated packets. I don't know if that is something that could event happen with a well-formed stream, but in combination with the previous bug this can easily cause a read access error.
• Very small NAL Units with only 2 bytes of nonzero data (Like a type 9 access unit delimiter) are not accepted as valid data and are discarded along with the remaining data of the frame.

In addition, I've noticed that the debug messages published every time a packet is received do have a significant negative performance impact when run in visual studio. I have not tested this with anything else.

Lastly, I've noticed that the first packet sent seems to always be accounted as corrupted by the receiver when sending in RTP_FORMAT_GENERIC. I have not taken the time to examine if that is due to uvgRTP or to my stream being malformed since this is not critical to my current project.

Have a nice day

I would recommend not using absolute paths as install destination like those:

 install(TARGETS uvgrtp
                ARCHIVE
                DESTINATION /usr/local/lib
        )

    install(DIRECTORY include/ DESTINATION /usr/local/include/uvgrtp
            FILES_MATCHING PATTERN "*.hh"
    )

Maye you could use something like this ?:

# Define install target, install libraries and archives (static libraries) to "<prefix>/lib"
install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}Targets
        LIBRARY DESTINATION lib
        ARCHIVE DESTINATION lib
        INCLUDES DESTINATION include)

#Copy all header files to the <prefix>/include/<projectName> directory with subdirectories retained
install(DIRECTORY ${CMAKE_SOURCE_DIR}/${PROJECT_NAME}
        DESTINATION include
        FILES_MATCHING PATTERN "*.hh")

Hi uvgRTP owners,

A recent issue on the subject "Would using more C++ features be welcome ?" calls out to me.

So I ask some questions :

• The uvgRTP is publicly released on GitHub, with a license that allows it to be used in industrial projects. Do the owners of uvgRTP want it to be used in this kind of projects? If the answer is yes, this has some involvement and responsability towards those people that use their project. This means that it can be used in long term developments, where some garantee is needed about its usability over time. Changing the uvgRTP build environment requirements in the mid time without backward compatibility is very damaging.

• What is the purpose of using more C++ features ? What is the added value ? C-ish code isn't bad by itself : a good code is a code that do well what it has been writen for and is understandable and maintainable. If the project maintainers are more comfortable with C-ish code style than with the last fashionable C++ constructs, I think it is better to maintain this style. The main goal is to have a well functionning library.

My fear is that a new C++ construct breaks the uvgRTP build with a C++11 compiler

As @db-tech pointed out in issue #12, it would be better to just add the private target directory and not use relative paths in includes.

Hi,

I have a problem with sending frames using RTP_FORMAT_GENERIC with the RCE_FRAGMENT_GENERIC flag set. It works fine with low-quality videos, but higher quality produces an error:

RCE_FRAGMENT_GENERIC splits the frame into ~1500 sized packets, and according to this error, the max number of chunks is 32. So the maximum allowed size of the frame is 1500*32 = 48000. Increasing the MTU solves this problem, but it is not realistic outside of the testing environment to increase MTU.

The same happens when I try to use HEVC encoding (I printed out the size of the frame):

Again, increasing the MTU solves this.

Is there a reason for the 32 part limit? What can I do to avoid this? Thanks

While working a little bit on #33 I've found some strange error return values on in different places:

        return -RTP_RECV_ERROR;
        return -RTP_RECV_ERROR;
        return -RTP_INVALID_VALUE;
        return -RTP_INVALID_VALUE;
        return -RTP_NOT_SUPPORTED;
        return -RTP_INVALID_VALUE;

Could this just be wrong / refactoring error or do i missunderstand something ? Then you can just close this.

I was refactoring zrtp, but I couldn't figure out what is the correct length of a zrtp message.

Most of zrtp messages calculate their length like this:

But then hello calculates it like this:

The difference is one byte I believe. In general this size should be in 32 bit words which means its incorrect regardless, but this difference is throwing me off even more.

Section 5.2 of RFC

All ZRTP messages begin with the preamble value 0x505a, then a 16-bit
length in 32-bit words.  This length includes only the ZRTP message
(including the preamble and the length) but not the ZRTP packet
header or CRC.  The 8-octet Message Type follows the length field.

It would suggest that the length is calculated in the same way for all. Some of the sizes are already give in the document.

Hello,

I am testing out the examples you've provided, and I am having a problem with parsing the output of the stream->pull_frame() function in this very first example(on the gray background): https://github.com/ultravideo/uvgRTP/tree/master/docs/examples#uvgrtp-example-codes.

I copied the code exactly, and instead of expected "Hello, world!" I am getting the following output:

Is this the intended output of the payload? If so, how do I get the plain "Hello, world!"? Thank you

Hello again,

This is more of a question than an issue, but I've been struggling with this for a while now. Consider I have code:

class myClass{
public:
    myClass(){
        session = ctx_.create_session("127.0.0.1");
        stream = session->create_stream(7777, 8888, RTP_FORMAT_GENERIC, 0);        
        (void)stream->get_rtcp()->install_receiver_hook(rtcpHook);//<- this is the problem, member function ptr
    }   
private:
    uvgrtp::context ctx;
    uvgrtp::session* session;
    uvgrtp::media_stream* stream;
    void rtcpHook(uvgrtp::frame::rtcp_receiver_report *frame){/body/}
}

I want to install a hook, which is a member function in this case. I have tried pretty much everything, like reinterpret casting, typedef, std::bind, etc. Nothing seems to correctly convert to the correct function pointer type. Could you explain to me how this is supposed to be done? Thanks

This check fails very easily for me:

At the minimum, the buffer should be increased significantly. An adaptive solution would also be nice.

#include <kvzrtp/lib.hh>
#include <QVideoFrame>

once I reverse the including order of two files:
#include <QVideoFrame>
#include <kvzrtp/lib.hh>

Errors disappear. Seems like something with macros define confliction.

I am using uvgRTP for a streaming application developped on CentOS7.9. Upgrading the OS is not an option.
Until now, I used the 1.0.0 release. I had to modify the random.cc file (more on this later) but the build succedeed.
I now want to upgrade to the 2.0.0 release. Unfortunately, the Makefile generated by cmake3 hardcodes the C++ standard version to c++17. The gcc compiler of CentOS7.9 is 4.8.5, which supports only c++11.

Wouldn't it be possible to ensure uvgrtp compiles with c++11 ?

The only thing that is specific to c++17 is located in the include/crypto.hh file.

I suggest to replace :
#if __has_include(<cryptopp/aes.h>) &&
...
!defined(RTP_NO_CRYPTO)

#define RTP_CRYPTO

#include <cryptopp/aes.h>
...
#endif

with :
#ifndef RTP_NO_CRYPTO
#include <cryptopp/aes.h>
...
#endif

The second point that is not available on CentOS7 is the getrandom() function.
I suggest to use the SYS_getrandom system call if the getrandom() function isn't available (HAVE_GETRANDOM undefined).
I have attached the modified file random.cc
random.txt

The length field is described as being measured in 32-bit words minus one, but this does not seem to be the case. All references to this length should be checked and fixed:

I would also recommend checking that the sizes actually are measured in 32-bit words, as they should be.

See: https://datatracker.ietf.org/doc/html/rfc3550#section-6.4.1

There are several places in uvgRTP where the code resembles more C than C++

This includes

• Lack of smart pointers (unique_ptr mostly)
• C style struct usage
• #defines instead of constexpr or const variables
• Initializing and defining local variables closer to their usage
• Change single comment lines to C++ single line comments (//)

These should be fixed at some point.

In the Documentation file (https://github.com/ultravideo/uvgRTP/blob/master/docs/README.md#public-api) the chapter "Public API" has a broken link to the API documentation. You should replace the .com extension with .io extension.

Fraction on this line:

This concept of RTCP received bytes is not found in the RFC and should be removed:

Hello,

I am testing the RTCP functionality based on the example (https://github.com/ultravideo/uvgRTP/blob/master/docs/examples/rtcp_hook.cc). I modified it slightly so that it only sends 5 frames. Also, I added the deallocation of the session object, as instructed here:
.

My issue is that the RTCP packets are terribly out of sync, and even when we go out of scope and start doing something else, the RTCP receiver reports keep on coming. Because we sent 5 frames, I expect 5 reports, but if we let it play out we receive much more than that:

Moreover, after we stop sending stuff, we just receive RTCP report of the last frame over and over. Inevitably, as we destroy the session, we get an error, because RTCP packets keep on coming:

void receiver_hook(uvgrtp::frame::rtcp_receiver_report* frame)
{
	LOG_INFO("Received an RTCP Receiver Report");

	for (auto& block : frame->report_blocks) {
		fprintf(stderr, "ssrc:      %x\n", block.ssrc);
		fprintf(stderr, "fraction:  %u\n", block.fraction);
		fprintf(stderr, "lost:      %d\n", block.lost);
		fprintf(stderr, "last_seq:  %u\n", block.last_seq);
		fprintf(stderr, "jitter:    %u\n", block.jitter);
		fprintf(stderr, "lsr:       %u\n", block.lsr);
		//fprintf(stderr, "dlsr (ms): %u\n", uvgrtp::clock::jiffies_to_ms(block.dlsr));
	}

	/* RTCP frames can be deallocated using delete */
	delete frame;
}

void appHook(uvgrtp::frame::rtcp_app_packet* packet) {
	LOG_INFO("Received an APP packet!!");
	fprintf(stderr, "Message: '%s'", packet->payload);
        delete packet;
}

int main() {

	uvgrtp::context ctx;
	uvgrtp::session* sess = ctx.create_session("127.0.0.1");
	uvgrtp::media_stream* sender = sess->create_stream(7777, 8888, RTP_FORMAT_GENERIC, RCE_RTCP);
	uvgrtp::media_stream* receiver = sess->create_stream(8888, 7777, RTP_FORMAT_GENERIC, RCE_RTCP);

	char* message = (char*)"Hello, world!";
	size_t len = strlen(message) + 1;
	auto data = (uint8_t*)message;
	
	char* name = (char*)"name";
	char* packet_txt = (char*)"pack";
	size_t pack_len = strlen(packet_txt) + 1;
	auto pack_data = (uint8_t*)packet_txt;
	
	
	//sender->get_rtcp()->install_app_hook(appHook);

	sender->get_rtcp()->install_receiver_hook(receiver_hook);

	uvgrtp::frame::rtp_frame* frame;
	for (int i = 0; i < 5; ++i) {
		sender->push_frame(data, len, 0);
		std::this_thread::sleep_for(std::chrono::milliseconds(2000));

		frame = receiver->pull_frame();
		fprintf(stderr, "RECEIVED Message: '%s'\n", frame->payload);
		

		//receiver->get_rtcp()->send_app_packet(name, 0, pack_len, pack_data);

		uvgrtp::frame::dealloc_frame(frame);

	}
	// uncomment to recreate repetitive RTCP packets
	/*
	for (int i = 0; i < 10000; ++i) {
		std::this_thread::sleep_for(std::chrono::milliseconds(20));
	}
	*/
	
	ctx.destroy_session(sess);
	return 0;
}

rtcp_app hook and manual APP packet sending will demonstrate how synchronous packet exchange could be.

Thank you

It seems that there is a bug in CMakeLists.txt.
Installation rules for header files doesn't work since the header source path is set wrong.

You should use "include/" instead of "src/" source path to install header files.

Please replace /src directory with include/ in the CMakeLists.txt as follows:
install(DIRECTORY include/ DESTINATION ${PROJECT_BINARY_DIR}/include
FILES_MATCHING PATTERN "*.hh"

As I was working an the other Issues, i thought it would be nice to have a .clang-format file.
Nowadays, that is almost the first thing I do in a new project.
I tried to create a .clang-format file that exactly matches the current style but hit a problem.
It is currently not possible to indent the class access modifier as it's own entity.
So, something like this cannot be done (at the moment, clang < version 12):

class A {
    public:
        int do_something();
        
};

What could be done is to indent the function definition with 4 spaces and offset the access modifier with 1 or two:

class A {
  public:
    int do_something();
        
};

I honestly would prefer it that way because I think the four spaces here (8 spaces from class to function definition) is a little to much. But because that changes the style you chose, I would understand that you disagree!

There is a good explanation of that problem here: https://stackoverflow.com/questions/29198963/how-can-i-tell-clang-format-to-indent-visibility-modifiers

Any thoughts ?

I create sessions like this
sess_ = ctx_.create_session("127.0.0.1");
send_ = sess_->create_stream(8888, 8889, RTP_FORMAT_GENERIC, RCE_FRAGMENT_GENERIC);
recv_ = sess_->create_stream(8889, 8888, RTP_FORMAT_GENERIC, RCE_FRAGMENT_GENERIC);

and the type of frame_s_ is const QVideoFrame &frame. I can receive frames by pull method, and reconstruct a QVideoFrame structure successfully. But it seems like the push_frame function disable the deallocation of videoframe buffer somehow. The program accumulates the memory by step of 0.9MB. That's the size of one frame.
I'm not clear about what's going on inside of push_frame function. Can u give some advices about how does push_frame funcition make the deallocation of frame not work?

Currently zrtp hangs after changing return values in 364c4d7. I don't think the commit is the problem, but the logic seems to be intended to block forever. I'll take a look at it tomorrow.

I just saw this code in h26x.cc

if (t1 && t4) {
                /* previous dword 0xxxxx0000 and current dword is 0x0001XXXX */
                if (t4) {
                    start_len = 4;
                    data[rpos] = lb;
                    return pos + 2;
                }
            /* Previous dwod was 0xXXXXXX00 */
            }

The second if if (t4) will always be true.

Is this just a useless if or could it be that this was meant to be a Bitwise & not a logical && ?
Keep in mint, I had no time to look of the logic of that code, just wanted to quickly report it so I don't forget it tomorrow.

I have added initial guide to CONTRIBUTING.md, but it could be improved so contributions to project are easier and the style is maintained better.

Part of the style would be maintained by .clang filed in issue #27 and those part would not have to be specified.

This issue should probably be done after #48

As I am checking the commit decde85 for the issue #12 I saw a lot of code like this:

if (!(pkt_dispatcher_ = new uvgrtp::pkt_dispatcher()))
        return free_resources(RTP_MEMORY_ERROR);

I am not sure I understand what this is supposed to do ?

Bug Description

The ZRTP fails around 40% in localhost, which seems concerning. I think the ZRTP protocol should be robust enough not to fail consistently in certain situations. Timing seems to affect this bug (how fast each side is started) but I'm not certain. The ZRTP should also be tested repeatedly over other types of connections. I have indications that ZRTP can also fail when used over the internet, but it is possible that this is a different issue.

Platforms

I have replicated this both on Windows and Linux.

How to replicate

This can be replicated by running the ZRTP multistream example or by running the automated tests. Both fail sometimes.

Solution

I'm not familiar enough with the protocol to say exactly how to fix this issue, other than trying to make the protocol implementation more robust somehow. One thing to look at is the mutexes in media_stream and whether they can lock one of the threads out while other is performing the ZRTP and failing.

I've previously traced it to uvgrtp::zrtp::begin_session() timeout at this line:

I tried increasing timeout and number of hello attempts, but this did not help. It clearly waited longer, but still did not manage to complete ZRTP.

Recent improvements to reception architecture seem to have mitigated the issue somewhat from what it was before, but I can still replicate the issue.

Affected files

Basically anything with zrtp in its name/path and media_stream.cc

In my opinion there are too many namespaces. For such a small project, I think uvgrtp is the only namespace needed. Now there are many other namespaces such as formats, zrtp_msg etc..

As a sidenote, can we get rid of the old namespace uvg_rtp?

wouldn't it be nice to separate header files needed externally and those only needed internally?
Maybe put all internal cpp and header files needed only internally together into the src directory and the public header into include/uvgRTP ?

I have seen that the library relies heavily on the class definitions to declare structs and values, which causes unnecessary dependencies between components. This could be solved by moving definitions to a new header file that declares all the necessary structs. Most prominently this affects probably the zrtp, but also other modules could make use of this.

I did check the source with a static code analyzer and found lots of maybe minor problems that should be resolved.

Just to show some examples

condition is always false

uint32_t intra = INVALID_TS;            

if (intra != INVALID_TS && enable_idelay) {
__drop_frame(finfo, intra);
    finfo->dropped.insert(intra);
}

unreachable code

if (more) {
    aggr_pkt_info_.nalus.push_back(std::make_pair(data_len, data));
    return RTP_NOT_READY;
} else {
....
     if (more)
        return RTP_NOT_READY;
....
}

Also there are a lot of those checks:

    if (frame->csrc)
        delete[] frame->csrc;

It by no means critical, but could be changed easily and therefore make it more readable.

Those are just examples, there is a lot more stuff.
I guess those could be done in one bigger PR...

Here is my process in cygwin terminal after cloning the repo

rocke@DESKTOP-RRF6MJJ /cygdrive/c/Users/rocke/CLionProjects/Streamer/libs/uvgRTP
$ mkdir build && cd build

rocke@DESKTOP-RRF6MJJ /cygdrive/c/Users/rocke/CLionProjects/Streamer/libs/uvgRTP/build
$ cmake ..
-- The C compiler identification is GNU 10.2.0
-- The CXX compiler identification is GNU 10.2.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc - works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++.exe
-- Check for working CXX compiler: /usr/bin/c++.exe - works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /cygdrive/c/Users/rocke/CLionProjects/Streamer/libs/uvgRTP/build

rocke@DESKTOP-RRF6MJJ /cygdrive/c/Users/rocke/CLionProjects/Streamer/libs/uvgRTP/build
$ ninja
ninja: error: loading 'build.ninja': No such file or directory  

rocke@DESKTOP-RRF6MJJ /cygdrive/c/Users/rocke/CLionProjects/Streamer/libs/uvgRTP/build
$ ls
CMakeCache.txt  CMakeFiles  Makefile  cmake_install.cmake

# System info

$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-cygwin/10/lto-wrapper.exe
Target: x86_64-pc-cygwin
Configured with: /mnt/share/cygpkgs/gcc/gcc.x86_64/src/gcc-10.2.0/configure --srcdir=/mnt/share/cygpkgs/gcc/gcc.x86_64/src/gcc-10.2.0 --prefix=/usr --exec-prefix=/usr --localstatedir=/var --sysconfdir=/etc --docdir=/usr/share/doc/gcc --htmldir=/usr/share/doc/gcc/html -C --build=x86_64-pc-cygwin --host=x86_64-pc-cygwin --target=x86_64-pc-cygwin --without-libiconv-prefix --without-libintl-prefix --libexecdir=/usr/lib --with-gcc-major-version-only --enable-shared --enable-shared-libgcc --enable-static --enable-version-specific-runtime-libs --enable-bootstrap --enable-__cxa_atexit --with-dwarf2 --with-tune=generic --enable-languages=c,c++,fortran,lto,objc,obj-c++ --enable-graphite --enable-threads=posix --enable-libatomic --enable-libgomp --enable-libquadmath --enable-libquadmath-support --disable-libssp --enable-libada --disable-symvers --with-gnu-ld --with-gnu-as --with-cloog-include=/usr/include/cloog-isl --without-libiconv-prefix --without-libintl-prefix --with-system-zlib --enable-linker-build-id --with-default-libstdcxx-abi=gcc4-compatible --enable-libstdcxx-filesystem-ts
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.2.0 (GCC)


$ cygcheck.exe -V
cygcheck (cygwin) 3.1.7
System Checker for Cygwin
Copyright (C) 1998 - 2020 Cygwin Authors
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ uname -m
x86_64

I built it on an Ubuntu machine and it worked okay, unfortunately it's not the machine I develop on anymore. Did I do something wrong?