GNOME Bugzilla – Bug 745187
JPEG2000 RTP video streaming problem
Last modified: 2016-05-13 15:30:01 UTC
Created attachment 297925 [details] rtp plugin details JPEG2000 video streaming over RTP using rtpj2kpay/rtpj2kdepay plugins doesn't seem to work. Using these command lines: (Server chain) gst-launch-1.0 -v filesrc location=test_qvga.mjpeg ! 'image/jpeg,width=320,height=240,framerate=25/1' ! jpegparse ! omxmjpegdec ! openjpegenc ! rtpj2kpay ! udpsink host=192.168.1.44 port=5000 (Client chain) gst-launch-1.0 -v udpsrc caps="application/x-rtp, media=(string)video, clock-rate=(int)90000, encoding-name=(string)JPEG2000, payload=(int)96, framerate=(fraction)25/1" port=5000 ! rtpj2kdepay ! fakesink dump=1 doesn't produce any packet dumps on the client. Removing the rtpj2kdepay element from the client chain does show that packets are being received. Running the client with extra debug info, like this: gst-launch-1.0 -v --gst-debug=rtpj2kdepay:5 udpsrc caps="application/x-rtp, media=(string)video, clock-rate=(int)90000, encoding-name=(string)JPEG2000, payload=(int)96, framerate=(fraction)25/1" port=5000 ! rtpj2kdepay ! fakesink dump=1 shows a stream of messages about discarded packets: ... rtpj2kdepay gstrtpj2kdepay.c:480:gst_rtp_j2k_depay_process:<rtpj2kdepay0> MHF 0, tile 0, frag 0, expected 1380 rtpj2kdepay gstrtpj2kdepay.c:488:gst_rtp_j2k_depay_process:<rtpj2kdepay0> discont of -1380, clear PU rtpj2kdepay gstrtpj2kdepay.c:555:gst_rtp_j2k_depay_process:<rtpj2kdepay0> discard packet, no sync ... To eliminate networking issues, I performed this test on the client: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink It doesn't work, if I run it with --gst-debug=rtpj2kdepay:5, I get the same stream of messages about discarded packets. Removing the rtpj2kpay/rtpj2kdepay pair "resolves" the problem. This command works as expected: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! openjpegdec ! videoconvert ! ximagesink As opposed to JPEG2000 video stream, this command with plain MJPEG over RTP works just fine: gst-launch-1.0 -v videotestsrc pattern=ball ! jpegenc ! rtpjpegpay ! rtpjpegdepay ! jpegdec ! videoconvert ! ximagesink I noticed these two lines when running the local test on client using JPEG2000: /GstPipeline:pipeline0/GstRtpJ2KDepay:rtpj2kdepay0.GstPad:src: caps = image/x-jpc, framerate=(fraction)0/1, fields=(int)1, colorspace=(string)sYUV /GstPipeline:pipeline0/GstOpenJPEGDec:openjpegdec0.GstPad:sink: caps = image/x-jpc, framerate=(fraction)0/1, fields=(int)1, colorspace=(string)sYUV where the framerate is apparently set to 0, but I don't see a way to change that.
Created attachment 297926 [details] server and client output
Created attachment 297927 [details] detailed log from the client
Created attachment 297928 [details] client local test output
Hi Miroslav, Did you solve this problem? Thanks, Aaron
(In reply to boxerab@gmail.com from comment #4) > Hi Miroslav, > Did you solve this problem? > Thanks, > Aaron No, although I never spent much time on it after reporting it. Moved on to using plain MJPEG for the application.
OK, thanks for the update.
I've fixed a number of issues with the payload methods, relating to tiled images. Payload followed by depayload seems to be correct for OpenJPEG test suite. Patch will be coming soon.
Created attachment 327551 [details] [review] Improve rtp j2k payloading of tiled images This patch fixes a number of issues for tiled images, including: 1) handling of fragmented main headers 2) storing correct tile index in rtp j2k header I also improved code commenting and variable naming. I tested payload followed by unpayload followed by decode on the OpenJPEG test suite, and all of the .j2c images decoded correctly
Miroslav, do you have time to test this patch?
Review of attachment 327551 [details] [review]: Thanks for the patch :) Would it be possible to split it up into multiple, functionally independent commits? It seems like containing several changes, like for one the consistent usage of the enum you put into the new header file, the moving of the enum to the header, the handling of the fragmented main header, the tile index fix, and maybe more. Having it split up will make it easier to review and helps getting it merged faster :) Also the commit message should have the format element/or/plugin: short description [empty line] long description of what is done in the commit unless the short description is already enough. [empty line] Link to the bugzilla ticket ::: gst/rtp/gstrtpj2kcommon.h @@ +1,2 @@ +/* GStreamer */ + Should have the same license boilerplate at the top as the other files @@ +27,3 @@ + + +#define RTP_J2K_HEADER_SIZE 8 GST/Gst namespace here would be useful
Yes, good idea. Will be submitting patches shortly.
Created attachment 327608 [details] [review] gstrtpj2k: move common code into shared header
Created attachment 327609 [details] gstrtpj2kpay: correctly manage header framentation, tile invalidation bit, and tile index
Sebastian, I've simplified my patch considerably, and broken it up into two.
Created attachment 327618 [details] [review] gstrtpj2kpay: correctly manage header framentation, tile invalidation bit, and tile index Fixed some issues with my previous patch
Review of attachment 327608 [details] [review]: ::: gst/rtp/gstrtpj2kcommon.h @@ +41,3 @@ + J2K_MARKER_SOD = 0x93, + J2K_MARKER_EOC = 0xD9 +} RtpJ2KMarker; The enum type and its values should also be properly namespaced ::: gst/rtp/gstrtpj2kdepay.c @@ +25,3 @@ + * and RFC 5372. + * For detailed information see: https://datatracker.ietf.org/doc/rfc5371/ + * and https://datatracker.ietf.org/doc/rfc5372/ Can you put the documentation additions into a separate commit? ::: gst/rtp/gstrtpj2kpay.c @@ +31,3 @@ + * a JPEG 2000 tile-part header, or a JPEG 2000 packet. + * + * The standard recommends that headers be put in their own RTP packets. And these. Also why is it noteworthy for the documentation that headers are recommended to be put into their own RTP packets? :)
Review of attachment 327618 [details] [review]: Generally looks good ::: gst/rtp/gstrtpj2kpay.c @@ +262,3 @@ + will make T invalid. + This cannot happen in our case since we always + send tile headers in their own RTP packets, so we cannot mix Should the sending of headers in their own packet maybe be a property? Might not always be wanted (more overhead) @@ +487,3 @@ + /* set MHF to zero if there is no more main header to process */ + if (state.header.MHF & 2) + state.header.MHF = 0; Please run gst-indent over your commits
(In reply to Sebastian Dröge (slomo) from comment #17) > Review of attachment 327618 [details] [review] [review]: > > Generally looks good Thanks > > ::: gst/rtp/gstrtpj2kpay.c > @@ +262,3 @@ > + will make T invalid. > + This cannot happen in our case since we always > + send tile headers in their own RTP packets, so we cannot mix > > Should the sending of headers in their own packet maybe be a property? Might > not always be wanted (more overhead) I don't think the overhead is significant: most of the space is taken up by the packet stream. Can we make this an enhancement for a later date? I just want to patch the defects for this ticket. > > @@ +487,3 @@ > + /* set MHF to zero if there is no more main header to process */ > + if (state.header.MHF & 2) > + state.header.MHF = 0; > > Please run gst-indent over your commits Will do.
(In reply to Sebastian Dröge (slomo) from comment #16) > Review of attachment 327608 [details] [review] [review]: > > ::: gst/rtp/gstrtpj2kcommon.h > @@ +41,3 @@ > + J2K_MARKER_SOD = 0x93, > + J2K_MARKER_EOC = 0xD9 > +} RtpJ2KMarker; > > The enum type and its values should also be properly namespaced Will do. > > ::: gst/rtp/gstrtpj2kdepay.c > @@ +25,3 @@ > + * and RFC 5372. > + * For detailed information see: https://datatracker.ietf.org/doc/rfc5371/ > + * and https://datatracker.ietf.org/doc/rfc5372/ > > Can you put the documentation additions into a separate commit? > Sure > ::: gst/rtp/gstrtpj2kpay.c > @@ +31,3 @@ > + * a JPEG 2000 tile-part header, or a JPEG 2000 packet. > + * > + * The standard recommends that headers be put in their own RTP packets. > > And these. Also why is it noteworthy for the documentation that headers are > recommended to be put into their own RTP packets? :) I put this in the documentation because this is a design decision - it means that certain scenarios are not possible. So, for future code reviewers, I think it is important for them to know this. Perhaps I should move it to the code, rather than put it in the documentation at the top.
Created attachment 327657 [details] [review] gstrtpj2k: update documentation
Created attachment 327658 [details] gstrtpj2k: move common code to shared header, code clean up
Created attachment 327659 [details] [review] gstrtpj2kpay: manage T tile invalidation bit correctly, manage fragmented headers correctly, update tile id in header correctly.
Created attachment 327660 [details] [review] gstrtpj2k: move common code to shared header, code clean up
(In reply to boxerab@gmail.com from comment #18) > > ::: gst/rtp/gstrtpj2kpay.c > > @@ +262,3 @@ > > + will make T invalid. > > + This cannot happen in our case since we always > > + send tile headers in their own RTP packets, so we cannot mix > > > > Should the sending of headers in their own packet maybe be a property? Might > > not always be wanted (more overhead) > > I don't think the overhead is significant: most of the space is taken up by > the packet stream. Can we make this an enhancement for a later date? > I just want to patch the defects for this ticket. Sure, just seemed like a good idea :) You can add a TODO comment in the code maybe
(In reply to boxerab@gmail.com from comment #19) > > ::: gst/rtp/gstrtpj2kpay.c > > @@ +31,3 @@ > > + * a JPEG 2000 tile-part header, or a JPEG 2000 packet. > > + * > > + * The standard recommends that headers be put in their own RTP packets. > > > > And these. Also why is it noteworthy for the documentation that headers are > > recommended to be put into their own RTP packets? :) > > > I put this in the documentation because this is a design decision - it means > that certain scenarios are not possible. So, for future code reviewers, I > think it is important for them to know this. Perhaps I should move it to the > code, rather than put it in the documentation at the top. Does it have effects for the user of the elements? What scenarios are not possible with that? If it's important for the user to know, it should be there. If only for developers working on the element itself, then it should be somewhere as a comment in the code.
Created attachment 327673 [details] [review] update to main patch for this bug handle multiple tile parts in single RTP packet, in case we enable this in the future
Comment on attachment 327673 [details] [review] update to main patch for this bug Looks good to me, but please split patches. You added another new feature inside this patch now (multi_tile_part), which could as well have been a patch on top of the previous one :) It's not a good idea to have one mega commit changing everything, as it will make it not only harder to review but also impossible to clearly bisect later to find a commit that introduced a bug.
Created attachment 327699 [details] [review] gstrtpj2kpay: manage fragmented headers correctly
Created attachment 327700 [details] [review] gstrtpj2kpay: manage T tile invalidation bit correctly, update tile id in header correctly.
commit f89c4f9f4b405c1ebc19041cb15087cefa738776 Author: Aaron Boxer <boxerab@gmail.com> Date: Thu May 12 08:43:39 2016 -0400 rtpj2kpay: manage T tile invalidation bit correctly, update tile id in header correctly. 1. according to RFC, T bit is only set when either the RTP packet only contains the J2K main header, or the packet contains tile parts from multiple tiles. This is now being managed correctly in the code. The second scenario cannot happen with our payloader, since tile headers are always placed in their own RTP packet, and so a packet cannot contain tile parts from multiple tiles. However, I have added code to track if multiple tile parts are included in a single RTP packet, in case in the future we want to put header and data in same packet. 2. Old code would set the tile id to zero for all J2K packets. This is now set correctly to the appropriate tile id. https://bugzilla.gnome.org/show_bug.cgi?id=745187 commit 84ff5511de550e3865afe06c2e1d3be78d7660ba Author: Aaron Boxer <boxerab@gmail.com> Date: Thu May 12 08:41:51 2016 -0400 rtpj2kpay: manage fragmented headers correctly J2K main header framentation across multiple RTP packets is now handled correctly https://bugzilla.gnome.org/show_bug.cgi?id=745187 commit d2765be1206e3866a1461221ac9dfa1cbe932814 Author: Aaron Boxer <boxerab@gmail.com> Date: Wed May 11 15:04:26 2016 -0400 rtpj2k: move common code to shared header, code clean up https://bugzilla.gnome.org/show_bug.cgi?id=745187 commit 82c2a5cbf830cf8706e2482875f9b7c2b8584fe3 Author: Aaron Boxer <boxerab@gmail.com> Date: Wed May 11 15:01:32 2016 -0400 rtpj2k: update documentation https://bugzilla.gnome.org/show_bug.cgi?id=745187
Thanks!
Very happy to make a small contribution to this great project; hope to make more in the future.