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

Bugs with RTP_FORMAT_H264 and RTP_FORMAT_GENERIC #6

Closed
jumarbach opened this issue Jan 18, 2021 · 5 comments
Closed

Bugs with RTP_FORMAT_H264 and RTP_FORMAT_GENERIC #6

jumarbach opened this issue Jan 18, 2021 · 5 comments

Comments

@jumarbach
Copy link

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

@altonen
Copy link
Collaborator

altonen commented Jan 19, 2021

Hello and thank you for your interest in uvgRTP

Unfortunately the H.264 was not tested thoroughly and it seems to be quite buggy. These aggregation packets are also a headache for H.265 implementation so if they are not absolutely crucial for your needs, I'd disable them and wait for a proper fix. Right now I don't have the hours to put into this project so I don't know when fixes for all these bugs will appear but I'll try to find time somewhere. For the time being, if these AVC fixes are crucial to you, you can replace the code in src/formats/h264.cc with the following to disable aggregation packet support. You can play with if (data_len <= 3) and if (data_len - 3 <= payload_size) checks to see if you can enable 2-byte NAL unit support.

rtp_error_t uvg_rtp::formats::h264::push_nal_unit(uint8_t *data, size_t data_len, bool more)
{
    if (data_len <= 3)
        return RTP_INVALID_VALUE;

    uint8_t nal_type    = data[0] & 0x1f;
    rtp_error_t ret     = RTP_OK;
    size_t data_left    = data_len;
    size_t data_pos     = 0;
    size_t payload_size = rtp_ctx_->get_payload_size();

    if (data_len - 3 <= payload_size) {
        if ((ret = fqueue_->enqueue_message(data, data_len)) != RTP_OK) {
            LOG_ERROR("Failed to enqueue Single NAL Unit packet!");
            return ret;
        }

        if (more)
            return RTP_NOT_READY;
        return fqueue_->flush_queue();
    }

Regarding these RTP_FORMAT_GENERIC issues, are you using the latest version of uvgRTP? I believe I fixed some problems relating to RCE_FRAGMENT_GENERIC but it's of course possible another bug slipped in. I will take a look at this too as soon as I can.

And lastly, the debug prints are very verbose and currently don't even serve much purpose which is a future task for somebody. Anyway, the debug prints can disabled with -DNDEBUG and all logging can be disabled with -D__RTP_SILENT__.

I'll close this issue when all these bugs have been fixed. Thanks for reporting these!

@jumarbach
Copy link
Author

Hello,
Sorry for the late answer. I am using the latest version of uvgRTP.
I actually have implemented workarounds around most of the issues, but I haven't tested them for more than my needs and I don't know if they even are conform to the rtp standard. But they do work for me, so this isn't really urgent from my perspective.

@altonen
Copy link
Collaborator

altonen commented Mar 24, 2021

Hi @jumarbach

I wanted to give you an update that the RTP_FORMAT_GENERIC issues have been resolved. I was not able to reproduce the issue where the first packet is rejected but after fixing the marker issue, both small and larger packets are sent and received successfully at least in my tests.

As for the H.264 fixes, I still haven't find time to finalize them. The fixes should be ready but the test framework I've been writing to validate the fixes is still in progress. It should be ready soon, I'll update you when those fixes are in the master.

@Thanh-Binh
Copy link

@altonen I use today RTP_FORMAT_GENERIC with RCE_FRAGMENT_GENERIC for big images.
I send firstly a small data (the image information like width, height, num_channels, image_type with the whole length 16bytes) to receiver.
I have the same problem that those data are discarded somehows.

@altonen
Copy link
Collaborator

altonen commented Apr 23, 2021

Hi,

I added the 2-byte NAL support and in my tests H.264 stream worked without issues. Unfortunately I'm unable to reproduce the issue with the generic interface but I fixed another issue there regarding fragment ordering. In case you or anyone reading this still encounters the issue where small frames are dropped when RCE_FRAGMENT_GENERIC is used, please open a new issue and please attach the code you're using and if possible, a link to a bitstream that can be used to reproduce the issue.

@altonen altonen closed this as completed Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants