GNOME Bugzilla – Bug 753323
mpegts: add support for JPEG 2000 to mpegtsmux and tsdemux
Last modified: 2017-08-03 06:28:26 UTC
Created attachment 308857 [details] [review] patch Support for JPEG 2000 in MPEG2 TS. Modified mpegtsmux and mpegtsdemux. Patch as attachment.
+1 for this feature.
May I ask what jpeg 2000 library is used in gstreamer? Thanks.
There's elements around openjpeg and libav's JPEG2000 codec.
Thanks, Sebastian.
Review of attachment 308857 [details] [review]: ::: gst/mpegtsdemux/tsdemux.c @@ +1110,3 @@ + case GST_MPEGTS_STREAM_TYPE_VIDEO_JP2K: + is_video = TRUE; + caps = gst_caps_new_empty_simple ("image/x-j2c"); Should be image/x-jpc most likely.
Comment on attachment 308857 [details] [review] patch Also the standard says that it should be an JPEG2000 elementary stream with some kind of header, not just a JPEG2000 code stream. That header has to be produced and stripped away / parsed. Take a look at the Opus support, you can probably do it the same way. Opus also has some custom, MPEG-TS specific header.
VSF document on J2K over MPEG-TS: http://www.videoservicesforum.org/activity_groups/VSF_TR-01_2013-04-15.pdf
PES prefix header is defined in Annex 5 of H.222.0 standard.
Created attachment 315432 [details] [review] changed j2c to jpc in previous patch changes based on discussions on IRC with slomo
Correction: PES prefix header is defined in Annex S of H.222.0 standard.
A polite <ping/> to the committers: would be nice to have this merged into master. Thanks!
Not before it complies to the standard, sorry :)
Hi, I'm trying add support for Jpeg2000 according to standard you provide. I also look at Opus support and ACC but it doesn't help me much. I follow Opus implementation and I'm bit confused what this peace of code do: if (stream->is_opus) { descriptor = gst_mpegts_descriptor_from_registration ("Opus", NULL, 0); g_ptr_array_add (pmt_stream->descriptors, descriptor); descriptor = gst_mpegts_descriptor_from_custom_with_extension (GST_MTS_DESC_DVB_EXTENSION, 0x80, &stream->opus_channel_config_code, 1); g_ptr_array_add (pmt_stream->descriptors, descriptor); It registers Opus as descriptor with 1B opus_channel_config_code value in descriptor->data pointer. But I found GST_MTS_DESC_J2K_VIDEO descriptor tag in (gstmpegtsdescriptor.h). Should I use st_mpegts_descriptor_from_custom(GST_MTS_DESC_J2K_VIDEO,...) instread ? And How can I pass my custom header data that I can access It in function similar to mpegtsmux_prepare_opus{jpeg2000} (it is ts_data->prepare_func callback) so I can set proper header values for elementary stream.
Hi Milos, Is your plugin code open source? It would help us find the problem if we could see the rest of the code. Also, are you able to make a JPEG 2000 over MPEG TS stream available? Thanks, Aaron
Created attachment 327498 [details] [review] Add elementary stream header I add support for Jpeg2000 to TS. It's missing correct elementary stream header data, but it should be working without it. The problem is that after I try demux TS(with JPEG2000), demuxer gives me (correct offset) but wrong stream->current_size value for first frame. Next stream->current_size values are correct but have wrong offset due to wrong current_size of first frame. I have no idea where the problem could be. Also I'm not sure what should be the descriptor registration name. So I put the name(value) of Jpeg2000 elementary stream (Found in Rec ITU-T T800) descriptor = gst_mpegts_descriptor_from_registration("elsm", NULL, 0); Even more I do not know for sure that stream->id = 0xBD; in function tsmux_stream_new is set according to standard. Standard says that it should be private stream.
Created attachment 327504 [details] [review] Add source files to Makefile.am an DRF_ID_JP2K to gstmpegdesc.h Forgot to add mpegtsmux_jpeg2000.c and mpegtsmux_jpeg2000.h to Makefile.am and also specifie DRF_ID_JP2K in gstmpegdesc.h
Thanks, Milos! What gstreamer pipeline are you using to test this? Also, do you have a sample stream?
Some comments on the patches: * Just make two patches, one for the demuxer, one for the muxer. * Don't make "fix the previous patch", make them patches which are self-contained * The demuxer part has comments/todo, I'll assume it's definitely not ready * Use C comments (/* .... */) and not c++ ones (//...) * You should get the profile/level/framerate/size/... from the jp2k descriptor (J2k_video_descriptor, 2.6.80 in the mpeg-ts specifications). That would require creating a 3rd patch to add support for parsing/creating that descriptor in the mpeg-ts library (gst-libs/gst/mpegts/..). * Since elsm is part of the official jpeg2000 specification (Annex M of Rec. ITU-T T.800), I don't see why you would need to parse it in the demuxer, just push it as-is, decoders *should* be able to handle it, no ? * Please provide some sample streams to test the code with. They will most likely be too big to post here, so provide links to them.
Thanks for code notes, I will fix my code and provide new version of it. * You right the elsm is part of Annex M of Rec. ITU-T T.800 but in ISO_IEC_15444-1 which is specification of clean Jpeg2000 codestream there is no ES header specified. It doesn't make sense to me if you mux clean Jpeg2000 codestream's into TS and then demux it, the input and the output are different because you do not truncate the ES header. So the tsdemux element should not have image/x-jpc as output format but rather something like "video/x-ts" because image/x-jpc stands for clean J2k codestream. What do you think ? * As far as I know the only place where I can access the "profile/level/framerate/size/..." info, which should be in caps I gess e.g. framerate , is from mpegtsmux_create_stream function in mpegtsmux.c and I need this info in tsmux_stream_get_es_descrs function in tsmuxstream.c So How can I pass this info ? Can I create some kind of structure encapsulates all info I need to create ES header in TsMuxStream ? There are already variables like audio_sampling, audio_bitrate and audio_channels used only by AC3 format.
One more question to "That would require creating a 3rd patch to add support for parsing/creating that descriptor in the mpeg-ts library (gst-libs/gst/mpegts/..)." There already is GST_MTS_DESC_J2K_VIDEO descriptor type in gstmpegtsdescriptor.c Is it what you mean by adding support for that descriptor ?
Note: VLC seems to have support for receiving J2K over MPEG -TS. See vlc/modules/demux/mpeg/ts.c file for details.
https://github.com/videolan/vlc/blob/master/modules/demux/mpeg/ts.c
Also note: there is free version of the standard (2012, superseded) available here: https://www.itu.int/rec/T-REC-H.222.0-201206-S/en J2K details are in Ammendment 5 of this document.
correction: annex S describes J2K details
Page 104 of this document describes J2k video descriptor
Can you please provide mpegts/j2k samples for this ?
So, VLC has a J2K over MPEG TS demuxer; I am working with them to create a muxer https://trac.videolan.org/vlc/ticket/16981 When this is in place, we can get some sample streams, and also spread the knowledge to other projects such as gstreamer and ffmpeg.
(In reply to Milos Seleceni from comment #19) > Thanks for code notes, I will fix my code and provide new version of it. Do you have a new version of the code, for review ? Also, a sample stream with ELSM would be very helpful. Thanks!
Hi, I've make progress with demuxer but it's not yet finished. I will push the code as soon as I clean the code. Yes I have an ELSM sample, but unfortunately It's private property and I do not have rights to share it
(In reply to Milos Seleceni from comment #29) > Hi, I've make progress with demuxer but it's not yet finished. I will push > the code as soon as I clean the code. Glad to hear this. I would recommend that you submit patches as early as you can, as there can be a lot of revisions until the patch is ready to be committed to the repository. Also, this makes it easier for us to troubleshoot your pipelines. > > Yes I have an ELSM sample, but unfortunately It's private property and I do > not have rights to share it I think I can find a sample that I can share.
Hi, as I was working on demuxer especially parsing J2k video descriptor I found out that my j2K-TS samples are not compatible with T-REC H222.0 standard (the table on page 104) My horizontal_size and vertical_size are stored at 16b instead of 32b and also I'm getting many warnings while parsing the rest of the file 0:00:00.023112000 29695 0x7fb88282e0f0 WARN pesparser pesparse.c:399:mpegts_parse_pes_header: Wrong packet start code 0xff64000e != 0x000001xx 0:00:00.023125000 29695 0x7fb88282e0f0 WARN tsdemux tsdemux.c:2155:gst_ts_demux_parse_pes_header: Error parsing PES header. pid: 0x65 stream_type: 0x21 Do you find a any samples, so we can check that those errors and warnings are only in my samples ?
Hi Milos, (In reply to Milos Seleceni from comment #31) > Hi, as I was working on demuxer especially parsing J2k video descriptor I > found out that my j2K-TS samples are not compatible with T-REC H222.0 > standard (the table on page 104) My horizontal_size and vertical_size are > stored at 16b instead of 32b and also I'm getting many warnings while > parsing the rest of the file Interesting. > > 0:00:00.023112000 29695 0x7fb88282e0f0 WARN pesparser > pesparse.c:399:mpegts_parse_pes_header: Wrong packet start code 0xff64000e > != 0x000001xx > > 0:00:00.023125000 29695 0x7fb88282e0f0 WARN tsdemux > tsdemux.c:2155:gst_ts_demux_parse_pes_header: Error parsing PES header. pid: > 0x65 stream_type: 0x21 > > Do you find a any samples, so we can check that those errors and warnings > are only in my samples ? Let me see what I can find. It may take me some time, as I am focused on other projects at the moment. By the way, it would be great if you could update your patch to reflect your progress on the muxer/demuxer. Cheers, Aaron
Well, I've done some research, and it looks like kakadu demo app can create a broadcast stream. So, here are the steps needed to create a stream, starting with a bunch of TIFF files: (tools used: Ffmpeg, Graphics Magick and kdu_v_compress) //////////////////////////////////////////////////////////////////////// // rename all files in directory to foo followed by an increasing sequence of consecutive numbers $ c=0; ls -rt | while read -r file; do ((++c)); mv "${file}" "$(printf 'foo_%04d.%s' "$c" "${file#*.}")"; done // force resize of all images in directory into 2K resolution $ c=0; ls -rt | while read -r file; do ((++c)); gm convert -size 1920x1080! "${file}" -resize 1920x1080! "${file}"; done // batch convert tiff to 8 bit (FFmpeg didn't like 12 bit TIFF) $ c=0; ls -rt | while read -r file; do ((++c)); gm convert "${file}" -define tiff:bits-per-sample=8 "${file}"; done // create YUV video file from tiff sequence $ ffmpeg -i foo_%04d.tif -vf format=yuv444p12le 1920x1080x5x444.yuv //convert to broadcast format with elementary stream headers $ kdu_v_compress -i 1920x1080x5x444.yuv -o test.jpb -jpb_data 1,200 ////////////////////////////////////////////////////////////////////////////// Milos, let me know if this creates the correct broadcast stream.
Kakadu demo apps are freely downloadable for non-commercial usage: http://kakadusoftware.com/downloads/
Hi all, we made a great progress with tsmuxer and demuxer. We are compatible now with AJA HW devices for creating and decoding Mpeg-TS with Jpeg2000. Few issues left to finish this: 1) Mpeg-TS requires Broadcast profile and Broadcast profile main level in j2k descriptor. We also have an jpeg2000 encoder producing such J2k codestream. My question is how should I get this info? I can create special structure, holding this information, in output caps of our encoder, but what about other encoders? This is not the best way probably! Or add extra property for it in mpegtsmux plugin, it's only for jpeg2000 so adding extra property for it is not the best idea either. 2) How to handle interlace video. The standard requires this structure in single buffer [[ELSM header][1. j2k codestream][2. codestream]]. The jpeg2000 encoder should produce two buffer for interlace video then in mpegtsmux I need concat these two buffers into the one plus add ELSM header. This is the only way I can think of it. If the encoder produce single buffer with both interlace frames in it I won't be able to separate them later in mpegtsmux. If my idea is right do you have any hints how to get two frames in mpegtsmux plugin instead of one? I've implemented it similar to Opus. I add callback for it mpegtsmux.c: mpegtsmux_create_stream() ts_data->prepare_func = mpegtsmux_prepare_jpeg2000; ts_data->prepare_data = private_data; ts_data->free_func = mpegtsmux_free_jpeg2000; but it calls for every frame. I can't create my extra buffer for two frames and wait for two calls of my callback and then just mux it all together. 3) When the j2k codestream exceed the 65Kb the buffer after adding the ELSM header is later split into two PES packets instead of one which produce invalid Mpeg-TS stream. Thanks for any help and suggestions.
(In reply to Milos Seleceni from comment #35) > Hi all, we made a great progress with tsmuxer and demuxer. We are compatible > now with AJA HW devices for creating and decoding Mpeg-TS with Jpeg2000. Few > issues left to finish this: > > 1) Mpeg-TS requires Broadcast profile and Broadcast profile main level in > j2k descriptor. We also have an jpeg2000 encoder producing such J2k > codestream. My question is how should I get this info? I can create special > structure, holding this information, in output caps of our encoder, but > what about other encoders? This is not the best way probably! How are the profiles and levels for JPEG2000 defined? Are they also stored inside the codestream somehow? We should probably do the same here as for h264 and other codecs. Have profile and level in the caps, and helper functions for extracting those from the stream if possible. > 2) How to handle interlace video. The standard requires this structure in > single buffer > [[ELSM header][1. j2k codestream][2. codestream]]. The jpeg2000 encoder > should produce two buffer for interlace video then in mpegtsmux I need > concat these two buffers into the one plus add ELSM header. This is the only > way I can think of it. If the encoder produce single buffer with both > interlace frames in it I won't be able to separate them later in mpegtsmux. This seems suboptimal. GstVideoDecoder and GstVideoEncoder are assuming that one buffer corresponds to exactly one frame, and not just to a field. You'll run into various problems everywhere here if it is separate buffers. What information is missing later to be able to split them again? There are no markers or other information anywhere, and you would somewhere need to know the size of each field? Maybe a GstMeta here for the case of interlaced JPEG2000...
> How are the profiles and levels for JPEG2000 defined? The Broadcast profiles are specifid in "ISO/IEC 15444-1 Information technology — JPEG 2000 image coding system: Core coding system AMENDMENT 3: Profiles for broadcast applications" There are exactly 3 of them, but in Mpeg-TS is allowed only first "Broadcast Contribution Single Tile Profile" and then there are main_levels, 5 of them, which specifies the max bitrate > Are they also stored inside the codestream somehow? No, only in j2k descriptor
Hi Milos, Glad to hear you've made progress. If you can share a patch with us, it will be easier to advise on how to fix these issues, and perhaps help with the work. Cheers, Aaron
(In reply to Milos Seleceni from comment #37) > > How are the profiles and levels for JPEG2000 defined? > > The Broadcast profiles are specifid in "ISO/IEC 15444-1 Information > technology — JPEG 2000 image coding system: Core coding system AMENDMENT 3: > Profiles for broadcast applications" > > There are exactly 3 of them, but in Mpeg-TS is allowed only first "Broadcast > Contribution Single Tile Profile" and then there are main_levels, 5 of them, > which specifies the max bitrate > > > Are they also stored inside the codestream somehow? > No, only in j2k descriptor That's annoying :) But then we'll have to add them as optional caps fields I guess. Your main problem then is that based on some arbitrary stream you don't know which profile that is. That's not too useful. And would mean that you either forbid muxing a lot of streams that would be ok, or allow muxing streams that are not ok. I'd guess the latter is better and is also what is done elsewhere in such situations here. It's then the application's job to ensure that the stream is following those constraints. Maybe have it print a warning if the caps field for the profile/level is not given.
Sorry I was wrong! There is "capabilities" field in j2k codestream in which the main_profile level is stored. For example DCI profiles used this field too. This capabilities are used by decoder as hint or some meta that decoded need to know for proper decoding. We can used it as far the only one Broadcast profile is allowed the Broadcast Contribution Single Tile Profile we have everything we need.
That makes it better then. So it should be exactly like with h264, and the parser should also extract profile/level and put it into the caps.
Sebastian, do you think we need a new file type to hold j2k stream plus ELSM headers? Looking at the Kakadu sample video encoder, they support ".jpb" files for broadcast, that seem to be j2k + ELSM.
What's an ELSM header?
elementary stream (elsm) header contains video-related information for a j2k encoded frame. So, I think the elsem is simply added in front of the j2k main header as a j2k box. A still image decoder would just ignore the elsm box, but an mpeg ts demuxer would parse the elsm. See https://www.itu.int/rec/T-REC-H.222.0-201206-S/en Annex S Section S.2 and S.3
Does it contain anything that is not part of the caps anyway?
ELSM contains: j2k_frat – frame rate coding. j2k_brat – maximum instantaneous bit rate of the elementary stream. j2k_fiel – interleaved field coding. If the j2k_fiel is present, there shall be two contiguous codestreams present. j2k_tcod –time code and frame count information of the J2K video access unit. j2k_bcol – broadcast colour specification coding. for each frame. So, it looks like ELSM header is not just a subset of caps info. Also, this file format will be useful to serialize stream to disk, for testing.
Sounds like caps + maximum bitrate tag? What's missing otherwise?
Broadcast colour spec is chosen from the following: sRGB 0x01 Rec-601 0x02 Rec-709 0x03 CIE-LUV 0x04 CIE-XYZ 0x05 Rec-2020 0x06 SMPTE-2084 0x07 Are these colour spaces currently supported?
LUV, XYZ and 2084 not but if we want to support them for JPEG2000 we should support them in general.
Oh I forgot that parts of that are not merged yet, you want to look at bug #771376 for exposing all these things on compressed caps. And GstVideoColorMatrix, GstVideoColorPrimaries, GstVideoTransferFunction
Thanks.
Created attachment 338828 [details] [review] mpegtsmux add jpeg2000
Created attachment 338829 [details] [review] tsdemux add jpeg2000
Hi, I've just noticed that you already add gstjpeg2000parsers, we can add parsing the capabilities and it should works. What about the bitrate property? So far tsmux doesn't support null packets, so the bitrate property would be misleading as it doesn't works yet.
Díky, Miloš!
Yes, after a might struggle, we do have a jpeg2000 parser :) Feel free to add bitrate parsing - it wasn't needed before. Also, colour space may have to be extended to handle broadcast colour spec.
By null packets, do you mean empty j2k packets in pack stream ?
(In reply to Aaron Boxer from comment #55) > Díky, Miloš! Nemáš za čo.
(In reply to Aaron Boxer from comment #57) > By null packets, do you mean empty j2k packets in pack stream ? Yes empty packets (0xff) for constant bitrate. We already discussed it with Sebastian and it's not trivial task
I forgot to mention, that tsmuxer works only with j2k smaller than 65000 bytes. If the j2k is larger, then it's split between two PES packets, which is wrong! Every frame have to be in single PES packet.
Thanks. May I ask why you split the frame if larger than 65k ?
No I'm trying to fix it :) The mpegtsmux element does split the j2k buffer into several PES packets. As far as I know the video frames should have unbiased buffer size but it doesn't work.
I see. Well, if you need some help with this, let me know.
Created attachment 338938 [details] [review] mpegtsmux add jpeg2000 fixed missing part of code
Review of attachment 338828 [details] [review]: Code quality is very good. I think you are missing a header file: "mpegtsmux_jpeg2000.h" in your attachment
Created attachment 341034 [details] [review] Add JPEG2000 stream type support Just add JPEG2000 stream type in header tsmuxstream.h, plus few fields in header for later j2k parsing
Created attachment 341035 [details] [review] tsmux Add support for JPEG2000 Add HHMMSSFF in ELSM header, FF part still need to work because of this issue https://bugzilla.gnome.org/show_bug.cgi?id=774837
Created attachment 341036 [details] [review] tsdemux Add support for JPEG2000 Need to proper interlace handling, add bitrate property in tsmux plugins.
So, what's the status of this patch ? Milos, are you planning on submitting a complete patch for review ? Thanks, Aaron
Well, I've put all of these patches together, fixed some compile errors, and put into a branch on github https://github.com/GrokImageCompression/gst-plugins-bad/tree/753323-mpegts-j2k The code seems quite incomplete, unfortunately. spring/summer project: get this working correctly
Here is a summary of all supporting documentation and software for this feature: 1. VSF document on J2K over MPEG-TS: http://www.videoservicesforum.org/activity_groups/VSF_TR-01_2013-04-15.pdf 2. ELSM is part of Annex M of Rec. ITU-T T.800 3. VLC support for J2K over MPEG TS https://github.com/videolan/vlc/blob/master/modules/demux/mpeg/ts.c 4. A free version of the standard (2012, superseded) available here: https://www.itu.int/rec/T-REC-H.222.0-201206-S/en PES prefix header is defined in Annex S of H.222.0 standard. Page 104 of this document describes J2k video descriptor The Broadcast profiles are specified in "ISO/IEC 15444-1 Information technology — JPEG 2000 image coding system: Core coding system AMENDMENT 3: Profiles for broadcast applications" MPEG TS must use the "Broadcast Contribution Single Tile" profile, plus 5 possible main levels, specifying max bit rate. Profile is stored in "capabilities" field in j2k code stream 5. kakadu demo app can create a broadcast stream. (tools used: Ffmpeg, Graphics Magick and kdu_v_compress) //////////////////////////////////////////////////////////////////////// // rename all files in directory to foo followed by an increasing sequence of consecutive numbers $ c=0; ls -rt | while read -r file; do ((++c)); mv "${file}" "$(printf 'foo_%04d.%s' "$c" "${file#*.}")"; done // force resize of all images in directory into 2K resolution $ c=0; ls -rt | while read -r file; do ((++c)); gm convert -size 1920x1080! "${file}" -resize 1920x1080! "${file}"; done // batch convert tiff to 8 bit (FFmpeg didn't like 12 bit TIFF) $ c=0; ls -rt | while read -r file; do ((++c)); gm convert "${file}" -define tiff:bits-per-sample=8 "${file}"; done // create YUV video file from tiff sequence $ ffmpeg -i foo_%04d.tif -vf format=yuv444p12le 1920x1080x5x444.yuv //convert to broadcast format with elementary stream headers $ kdu_v_compress -i 1920x1080x5x444.yuv -o test.jpb -jpb_data 1,200 ////////////////////////////////////////////////////////////////////////////// Kakadu demo apps are freely downloadable for non-commercial usage: http://kakadusoftware.com/downloads/
ELSM contains: j2k_frat – frame rate coding. j2k_brat – maximum instantaneous bit rate of the elementary stream. j2k_fiel – interleaved field coding. If the j2k_fiel is present, there shall be two contiguous codestreams present. j2k_tcod –time code and frame count information of the J2K video access unit. j2k_bcol – broadcast colour specification coding. for each frame. Broadcast colour spec is chosen from the following: sRGB 0x01 Rec-601 0x02 Rec-709 0x03 CIE-LUV 0x04 CIE-XYZ 0x05 Rec-2020 0x06 SMPTE-2084 0x07
Test pipeline: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! mpegtsmux ! tsdemux ! openjpegdec ! videoconvert ! ximagesink
There seems to be a problem with interlaced video. The following test pipeline gst-launch-1.0 -v videotestsrc pattern=ball ! deinterlace ! openjpegenc ! mpegtsmux ! tsdemux ! openjpegdec ! videoconvert ! ximagesink fails negotiation with internal data stream error.
Created attachment 351325 [details] [review] Re-work of existing patches This patch obsoletes previous patches in this issue, as it includes all of them. The test pipeline mentioned in the issue is now working with these changes, so everything is looking good. Code is not quite ready to merge, but it would very helpful to have a code review to see what is missing. Thanks!
Created attachment 351326 [details] [review] Removed some C++ style comments
Created attachment 351341 [details] [review] removed j2k sampling from tsdemux: use jpeg20000parse to parse sampling
Working test pipeline: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink the parser is needed to set the sampling field for openjpegdec
Question about interlaced: since JPEG 2000 is an intra-frame codec, the standard requires that both interlace fields be part of a single buffer. From previous comments, it looks like this is tricky. Is there a way of getting the muxer to put both fields in single buffer ?
Hello Milos, In your patch, what do you think about changing the copyright of the mpegtsmux_jpeg2000 files, to put your name as copyright holder (or your company name). Currently it reads * Copyright 2006, 2007, 2008 Fluendo S.A. * Authors: Jan Schmidt <jan@fluendo.com> * Kapil Agrawal <kapil@fluendo.com> * Julien Moutte <julien@fluendo.com> which is not correct. Thoughts? Aaron
Review of attachment 315432 [details] [review]: This patch is now obsolete
Current test pipeline: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink
Note: this issue depends on #782337 https://bugzilla.gnome.org/show_bug.cgi?id=782337
gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink gives a SIGSEV. Currently using gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink
Review of attachment 308857 [details] [review]: This very old patch is now not needed anymore.
Review of attachment 351326 [details] [review]: obsolete
Review of attachment 351341 [details] [review]: obsolete
Created attachment 353329 [details] [review] jpeg2000parse: parse colorimetry in sink caps and add to source caps
Created attachment 353330 [details] [review] Add jpeg2000 support for mux and demux
Created attachment 353332 [details] [review] work-around fo SIGSEV when jpeg2000parse follows openjpegenc in pipeline The following pipeline throws a SIGEV gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink This workaround allows gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink to work - remove the jpeg2000 parser from the beginning of pipeline. This patch will be removed once the SIGSEV is fixed, but I am adding the patch so perhaps someone else can repro the SIGSEV
The current set of patches does not handle interlaced video. Problem with interlaced is that MPEG TS J2K spec requires both fields to be muxed in the same frame, and muxer is not designed to do this. For phase 1, can we just refuse to negotiate if we are getting interlaced ? At least we can get something working and tested for progressive.
BTW, these patches can also be found on my github repo: https://github.com/GrokImageCompression/gst-plugins-bad/tree/753323-mpegts-j2k
Latest commit on the branch fixes the SIGSEV
This pipeline is currently working with tip of my branch: gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink
(In reply to Aaron Boxer from comment #92) > For phase 1, can we just refuse to negotiate if we are getting interlaced ? > At least we can get something working and tested for progressive. Yes
Review of attachment 353329 [details] [review]: ::: gst/videoparsers/gstjpeg2000parse.c @@ +654,3 @@ } + Extra empty line not needed @@ +659,3 @@ + if (colorimetry_string) { + gchar *colorimetry = g_strdup (colorimetry_string); + if (colorimetry) { If colorimetry_string is not NULL, then colorimetery will also not be NULL here @@ +662,3 @@ + gst_caps_set_simple (src_caps, "colorimetry", G_TYPE_STRING, + colorimetry, NULL); + g_free (colorimetry); Why the g_strdup() and then g_free() again immediately?
Review of attachment 353332 [details] [review]: Why exactly does it segfault and what does this patch fix exactly and how? ::: gst/mpegtsmux/mpegtsmux.c @@ +786,3 @@ + gst_allocator_free (default_allocator, private_data); + gst_object_unref (default_allocator); + GST_ERROR_OBJECT (pad, "Missing JPEG 2000 profile"); Why so complicated and not just gst_memory_unref (private_data)?
Review of attachment 353330 [details] [review]: ::: gst/mpegtsdemux/tsdemux.c @@ +1494,3 @@ + DEN_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br); + NUM_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br); + color_specification = gst_byte_reader_get_uint8_unchecked (&br); You should check that you actually have that much data available instead of blindly reading @@ +1501,3 @@ + (void) vertical_size; + (void) max_bit_rate; + (void) max_buffer_size; G_GNUC_UNUSED seems better for that @@ +2664,3 @@ + GstBufferList *buffer_list = NULL; + + Double empty line @@ +2821,3 @@ + + buffer = gst_buffer_new_wrapped (packet_data, packet_size); + gst_buffer_list_add (buffer_list, buffer); Why a buffer list? Why could there be multiple buffers here (and would each buffer be a complete JPC frame?)? ::: gst/mpegtsmux/mpegtsmux.c @@ +758,3 @@ + j2k_private_data *map_data_p = NULL; + GstMemory *private_data = + gst_allocator_alloc (NULL, sizeof (j2k_private_data), NULL); Why a GstMemory here instead of just using g_malloc()? You're not sending the memory downstream in a buffer or similar @@ +764,3 @@ + GstAllocator *default_allocator = + gst_allocator_find (GST_ALLOCATOR_SYSMEM); + gst_allocator_free (default_allocator, private_data); gst_memory_unref() ::: gst/mpegtsmux/mpegtsmux_jpeg2000.c @@ +108,3 @@ + guint8 seconds = (guint8) (buf->pts / 1e9); + guint8 minutes = (guint8) (seconds / 60); + guint8 hours = (guint8) (minutes / 60); Won't these overflow the 8 bits usually? And instead of 1e9 better use GST_SECOND @@ +137,3 @@ + GST_DEBUG_OBJECT (mux, "Creating buffer of size: %lu", out_size); + + wr = gst_byte_writer_new (); Initialize on the stack, maybe also with some expected size @@ +140,3 @@ + + /* Elementary stream header box 'elsm' == 0x656c736d */ + if (gst_byte_writer_put_int32_be (wr, 0x656c736d)) { These generally can't fail, and if they do you want to go out immediately. Please reduce the level of indentation here and below :) @@ +278,3 @@ + /* Free prepare data memory object */ + GstAllocator *default_allocator = gst_allocator_find (GST_ALLOCATOR_SYSMEM); + gst_allocator_free (default_allocator, (GstMemory *) prepare_data); gst_memory_unref() ::: gst/mpegtsmux/mpegtsmux_jpeg2000.h @@ +3,3 @@ + * Authors: Jan Schmidt <jan@fluendo.com> + * Kapil Agrawal <kapil@fluendo.com> + * Julien Moutte <julien@fluendo.com> I don't think any of these people contributed to this new file, or the corresponding .c file :) ::: gst/mpegtsmux/tsmux/tsmuxstream.c @@ +798,3 @@ + guint8 level = stream->profile_and_level & 0xF; + guint32 max_buffer_size = 0; + GstByteWriter *writer = gst_byte_writer_new (); Initialize this on the stack ideally
Thanks a lot for the review! Working on these fixes :)
Created attachment 355044 [details] [review] jpeg2000parse: parse colorimetry and interlace mode in sink caps and add to source caps changes after review
Created attachment 355045 [details] [review] jpeg2000parse: parse colorimetry and interlace mode in sink caps and add to source caps added bugzilla url to commit message
Review of attachment 355045 [details] [review]: In addition, "field-order", "multiview-mode" and maybe "chroma-site" might also be useful ::: gst/videoparsers/gstjpeg2000parse.c @@ +25,3 @@ #include "gstjpeg2000parse.h" #include <gst/base/base.h> +#include <gst/video/video-color.h> Why?
Thanks for the review. Patch incoming.
Created attachment 355252 [details] [review] jpeg2000parse: parse colorimetry, interlace-mode, filed-order, multiview-mode and chroma-site
Created attachment 355254 [details] [review] jpeg2000parse: parse colorimetry, interlace-mode, filed-order, multiview-mode and chroma-site removed extra new-line :)
Created attachment 355279 [details] [review] Add jpeg2000 support for mux and demux Addresses all comments from review except for overflow in these lines + guint8 seconds = (guint8) (buf->pts / 1e9); + guint8 minutes = (guint8) (seconds / 60); + guint8 hours = (guint8) (minutes / 60); The video descriptor expects these fields as 8 bit quantities, so I don't think there is any way around the overflow.
Created attachment 355292 [details] [review] Add jpeg2000 support for mux and demux removed some indentation
Created attachment 355293 [details] [review] Add jpeg2000 support for mux and demux get rid of unnecessary buffer list
Comment on attachment 355254 [details] [review] jpeg2000parse: parse colorimetry, interlace-mode, filed-order, multiview-mode and chroma-site commit 85fbbc7a24f8c8080cb564995dc990c45bccc85c (HEAD -> master) Author: Aaron Boxer <boxerab@gmail.com> Date: Thu Jul 6 15:14:57 2017 -0400 jpeg2000parse: Parse colorimetry, interlace-mode, field-order, multiview-mode and chroma-site https://bugzilla.gnome.org/show_bug.cgi?id=753323
Review of attachment 355293 [details] [review]: ::: gst/mpegtsdemux/tsdemux.c @@ +1522,3 @@ + + + remaining_8b = gst_byte_reader_get_uint8_unchecked (&br); Many empty new-lines, and you should probably check if there actually is another uint8 to read here @@ +1558,3 @@ + default: + colorspace = ""; + colorimetry_mode = ""; In this case, don't set the fields to empty strings but leave them empty or don't create caps at all @@ +1751,3 @@ + if (name) + g_free (name); g_free() is NULL-safe @@ +2662,3 @@ + if (stream->data) { + g_free (stream->data); g_free() is NULL-safe @@ +2672,3 @@ GST_ERROR ("Failed to parse Opus access unit"); + if (stream->data) { + g_free (stream->data); g_free() is NULL-safe @@ +2676,3 @@ + } + stream->current_size = 0; + gst_buffer_list_unref (buffer_list); gst_buffer_list_unref() isn't NULL-safe @@ +2821,3 @@ + } + /* Get reserved 8-bits */ + if (!gst_byte_reader_get_uint8 (&reader, &b)) { Instead of all the checks, you could also use the get_XXX_unchecked() variants and in the beginning check once if you have enough data for everything @@ +2859,1 @@ g_free (stream->data); g_free() is NULL-safe ::: gst/mpegtsmux/mpegtsmux.c @@ +770,3 @@ + } + } else { + GST_ERROR_OBJECT (pad, "Missing JPEG 2000 profile"); You probably want to put the profile into the caps of the sink pad template then @@ +808,3 @@ + } else { + /*GST_ERROR_OBJECT (pad, "Missing main level"); + goto not_negotiated; */ And the same for the main level @@ +899,3 @@ + /* Width and Height */ + gst_structure_get_int (s, "width", &ts_data->stream->horizontal_size); + gst_structure_get_int (s, "height", &ts_data->stream->vertical_size); Should check if those fields actually exist, or require them on the sink pad template caps ::: gst/mpegtsmux/mpegtsmux_jpeg2000.c @@ +136,3 @@ + gst_byte_writer_put_uint8 (&wr, seconds); + gst_byte_writer_put_uint8 (&wr, 0x0); + /* private_data->frame_number++; */ Why commented out? @@ +140,3 @@ + /* private_data->HHMMSSFF */ + /* + if(!gst_byte_writer_put_int32_be(&wr, 0x00)) What is this commented out block? ::: gst/videoparsers/gstjpeg2000parse.c @@ +1,2 @@ /* GStreamer JPEG 2000 Parser + * Copyright (C) <2016-2017> Grok Image Compession Inc. Typo: CompRession
Thanks for the review! Working on a patch now. Keep in mind that I didn't write most of this code :) so there are some mysteries about commented out code.
Created attachment 355882 [details] [review] Add jpeg2000 support for mux and demux Changes from slomo's review. I think I've addressed all of the comments, except that I relex the spec's requirement that the j2k profile should be a broadcast profile, and that the main level should be present. Currently, I don't have a way of generating j2k frames with this info. Also, as discussed earlier, I have disabled interlace support - interlaced stream will error out.
Created attachment 355907 [details] [review] Add jpeg 2000 support for TS mux/demux a little more cleanup :)
Created attachment 355908 [details] [review] Add jpeg 2000 support to TS mux/demux more cleanup
Created attachment 355909 [details] [review] Add jpeg 2000 support for TS mux/demux a few more small improvements
Review of attachment 355909 [details] [review]: ::: gst/mpegtsdemux/tsdemux.c @@ +1483,3 @@ + guint desc_min_length = 24; + + if (desc->length < desc_min_length) { Why not a constant or integer literal? @@ +2686,3 @@ + int remaining = 0; + + if (stream->current_size < header_size) { Why not a constant or integer literal? ::: gst/mpegtsmux/mpegtsmux.c @@ +755,3 @@ + const GValue *vInterlace = gst_structure_get_value (s, "interlace-mode"); + const GValue *vColorimetry = gst_structure_get_value (s, "colorimetry"); + j2k_private_data *private_data = g_malloc (sizeof (j2k_private_data)); You seem to be leaking the private_data in error cases. Also better use g_new0(j2k_private_data, 1) here. @@ +757,3 @@ + j2k_private_data *private_data = g_malloc (sizeof (j2k_private_data)); + if (!private_data) { + GST_ERROR_OBJECT (pad, "Unable to allocate memory"); g_malloc(), g_new(), etc never return NULL. If allocation fails, they abort() the process. @@ +809,3 @@ + private_data->interlace = FALSE; /* will be set later. TODO: interlace video mode doesn't work yet */ + private_data->den = 0; /* will be set later */ + private_data->num = 0; /* will be set later */ Is a fixed framerate required, or can it be 0/1 or 0/0? @@ +812,3 @@ + private_data->max_bitrate = max_rate; + private_data->Fic = 1; /* TODO: get from caps */ + private_data->Fio = 0; /* TODO: get from caps */ What is the meaning of these? @@ +825,3 @@ + if (vInterlace) { + const char *interlace_mode = g_value_get_string (vInterlace); + private_data->interlace = g_str_equal (interlace_mode, "interleaved"); There's progressive, interleaved and mixed. None of these are "one field per buffer", all of them are "one frame / two fields per buffer". So I think as of now you can just handle interlaced like progressive. You will get both fields encoded into the same frame, interleaved. The special interlaced mode here is, AFAIU, for "one field per buffer"? @@ +842,3 @@ + color_spec = 3; + } else { + color_spec = 1; /* RGB as default */ What is it that is these values mean? Color range, primaries, matrix, gamma transfer function, all at once? You might want to use gst_video_colorimetry_from_string() and then look specifically at the struct returned ::: gst/mpegtsmux/mpegtsmux_jpeg2000.c @@ +48,3 @@ + guint8 seconds = (guint8) (buf->pts / GST_SECOND); + guint8 minutes = (guint8) (seconds / 60); + guint8 hours = (guint8) (minutes / 60); While these are only stored as guint8, you would overflow the seconds here needlessly (they are % 60 later anyway), which would give wrong values for the minutes. And same thing with minutes/hours. Best to store these in a guint64 during calculations, you make sure they're in the right range at the very end anyway but during the calculations it would be good to not go completely off because of integer overflows. @@ +62,3 @@ + + /* ??? Hack for missing frame number index in buffer offset */ + /* guint8 frame_number = private_data->frame_number % 60; */ You could just count frames, or not? Starting at 0, all frames that are muxed here. AFAIU the hours/minutes/seconds/frames are a timecode here though, so maybe better to create actual timecodes here instead of calculating something from the PTS. You can use the GstVideoTimeCode API for that: Either you get buffers with timecodes as input (as a meta on the buffers), or you just start counting at 0 and then increase with every buffer (or you leave it all 0 if there are no timecodes added upstream? is that valid?). Then you'll get valid values for hours/minutes/seconds/frames, which you might not by just taking the PTS. @@ +70,3 @@ + gst_byte_writer_put_int32_be (&wr, 0x656c736d); + /* Framerate box 'frat' == 0x66726174 */ + gst_byte_writer_put_int32_be (&wr, 0x66726174); Could also do _put_data(&wr, "frat", 4) if wanted, but doesn't really matter @@ +84,3 @@ + if (private_data->interlace) { + /* put size of second codestream */ + gst_byte_writer_put_int32_be (&wr, gst_buffer_get_size (buf)); This seems wrong for interlacing, maybe put a FIXME/TODO comment somewhere explaining what is missing for interlacing @@ +122,3 @@ + gst_buffer_map (buf, &buf_map, GST_MAP_READ); + gst_buffer_fill (out_buf, header_size, buf_map.data, buf_map.size); + gst_buffer_unmap (buf, &buf_map); Could also use gst_buffer_copy_into() here (or just combine it with the gst_buffer_copy_into() above). Set offset to header_size for that, and size to -1. ::: gst/mpegtsmux/tsmux/tsmuxstream.h @@ -248,1 @@ -guint64 tsmux_stream_get_pts (TsMuxStream *stream); Please don't run gst-indent over headers, only C/C++ source files
Created attachment 355975 [details] [review] JPEG 2000 add TS mux/demux Thanks for the review! I think I've addressed most of your comments. For fic and fio, I am not actually sure how they are supposed to be used; presumably to identify ciodstream one or two? But, since we only mux interleaved data, and don't support demuxing interlaced, I think we can leave this for now ? Also, I am not sure how to handle the frame number issue - Milos had some problems with the frame number, but I am not clear on what they were. Perhaps this was a rounding issue, which we have fixed by using 64 bit intermediate variables ? Unfortunately, we don't have another source to test with.
Created attachment 355976 [details] [review] JPEG 2000 add TS mux/demux A little clean up
(In reply to Sebastian Dröge (slomo) from comment #117) > > > @@ +809,3 @@ > + private_data->interlace = FALSE; /* will be set later. TODO: > interlace video mode doesn't work yet */ > + private_data->den = 0; /* will be set later */ > + private_data->num = 0; /* will be set later */ > > Is a fixed framerate required, or can it be 0/1 or 0/0? Not sure if frame rate is fixed. What does 0/1 and 0/0 mean ? > > @@ +812,3 @@ > + private_data->max_bitrate = max_rate; > + private_data->Fic = 1; /* TODO: get from caps */ > + private_data->Fio = 0; /* TODO: get from caps */ > > What is the meaning of these? I am not sure what Fix and Fio mean, but since we don't support "interlaced", it shouldn't matter at this stage. > > @@ +842,3 @@ > + color_spec = 3; > + } else { > + color_spec = 1; /* RGB as default */ > > What is it that is these values mean? Color range, primaries, matrix, gamma > transfer function, all at once? You might want to use > gst_video_colorimetry_from_string() and then look specifically at the struct > returned I think the color spec indicates the colour space.
(In reply to Aaron Boxer from comment #118) > Also, I am not sure how to handle the frame number issue - Milos had some > problems with the frame number, but I am not clear on what they were. > Perhaps this was a rounding issue, which we have fixed by using 64 bit > intermediate variables ? My point here is that this is a timecode, not a timestamp :) You need to use the GstVideoTimeCode API for doing anything meaningful here, which would also make any rounding issues disappear (In reply to Aaron Boxer from comment #120) > > Is a fixed framerate required, or can it be 0/1 or 0/0? > > > Not sure if frame rate is fixed. What does 0/1 and 0/0 mean ? 0/0 is invalid in GStreamer, but would be what you set here if there is no framerate provided by GStreamer. What does the spec say about this? 0/1 is GStreamer's way of saying "this is variable framerate". What does the spec say about variable framerate? Related to that, is the timecode box optional? Because timecodes make no sense for variable framerate streams > > > > @@ +842,3 @@ > > + color_spec = 3; > > + } else { > > + color_spec = 1; /* RGB as default */ > > > > What is it that is these values mean? Color range, primaries, matrix, gamma > > transfer function, all at once? You might want to use > > gst_video_colorimetry_from_string() and then look specifically at the struct > > returned > > I think the color spec indicates the colour space. What does the spec say exactly for these values? You probably want color primaries or the matrix here then.
Thanks for reply above. Working on reply. Here are a few other issues reported earlier, not sure how to resolve them: 1) When the j2k codestream exceed the 65Kb, the buffer after adding the ELSM header is later split into two PES packets instead of one which produces invalid Mpeg-TS stream. 2) tsmux doesn't support null packets, which is needed for constant bitrate streams
(In reply to Aaron Boxer from comment #122) > 1) When the j2k codestream exceed the 65Kb, the buffer after adding the ELSM > header is later split into two PES packets instead of one which produces > invalid Mpeg-TS stream. Why is it wrong to split the packets? What should be done instead according to the spec? Or should they be combined differently again? > 2) tsmux doesn't support null packets, which is needed for constant bitrate > streams That's a known limitation of tsmux, and for constant bitrate streams more than just that is needed
(In reply to Sebastian Dröge (slomo) from comment #121) > (In reply to Aaron Boxer from comment #118) > > > Also, I am not sure how to handle the frame number issue - Milos had some > > problems with the frame number, but I am not clear on what they were. > > Perhaps this was a rounding issue, which we have fixed by using 64 bit > > intermediate variables ? > > My point here is that this is a timecode, not a timestamp :) You need to use > the GstVideoTimeCode API for doing anything meaningful here, which would > also make any rounding issues disappear Thanks, will take a look. > > (In reply to Aaron Boxer from comment #120) > > > > Is a fixed framerate required, or can it be 0/1 or 0/0? From what I can see, frame rate is fixed, so 0/0 is not allowed. Also, frame rate of 0 is not allowed according to spec, so 0/1 also not allowed. > > > > > > Not sure if frame rate is fixed. What does 0/1 and 0/0 mean ? > > 0/0 is invalid in GStreamer, but would be what you set here if there is no > framerate provided by GStreamer. What does the spec say about this? > > 0/1 is GStreamer's way of saying "this is variable framerate". What does the > spec say about variable framerate? > > Related to that, is the timecode box optional? Because timecodes make no > sense for variable framerate streams Time code box is mandatory. So, pretty sure that framerate is fixed. From spec, range for time code is: HH (0-23) MM (0-59) SS (0-59) FF (1-60) > > > > > > > @@ +842,3 @@ > > > + color_spec = 3; > > > + } else { > > > + color_spec = 1; /* RGB as default */ > > > > > > What is it that is these values mean? Color range, primaries, matrix, gamma > > > transfer function, all at once? You might want to use > > > gst_video_colorimetry_from_string() and then look specifically at the struct > > > returned > > > > I think the color spec indicates the colour space. > > What does the spec say exactly for these values? You probably want color > primaries or the matrix here then. Colour spec is described in a different document that I don't have access to. But I do know the following: sRGB 0x01 Rec-601 0x02 Rec-709 0x03 CIE-LUV 0x04 CIE-XYZ 0x05 Rec-2020 0x06 SMPTE-2084 0x07
Color spec is described here: Annex M of Rec. ITU-T T.800 (2002)/Amd.3 | ISO/IEC 15444-1:2004/Amd.3 if anyone owns a copy, or has the 198 swiss francs to purchase it :)
Right, here it is! (a superseded version) http://www.itu.int/rec/T-REC-T.800-201006-S!Amd3
(In reply to Aaron Boxer from comment #124) > > (In reply to Aaron Boxer from comment #120) > > > > > > Is a fixed framerate required, or can it be 0/1 or 0/0? > > From what I can see, frame rate is fixed, so 0/0 is not allowed. Also, > frame rate of 0 is not allowed according to spec, so 0/1 also not allowed. So you need to require a framerate in the caps, and it must be higher than 0/1. And this also makes it required that the input is actually fixed framerate. Seems like a weird requirement for MPEG-TS... are you sure about that? Or maybe the framerate is only an indication, not necessarily to be correct? > > Time code box is mandatory. So, pretty sure that framerate is fixed. > From spec, range for time code is: > > HH (0-23) > MM (0-59) > SS (0-59) > FF (1-60) Ok, so you must invent your own timecode with the API if there is no meta with an actual timecode on the stream. Or you error out directly if there is no timecode meta, and require the user to use the timecodestamper element. I think I prefer requiring timecodestamper. You also require videorate to be used for a fixed framerate. > > What does the spec say exactly for these values? You probably want color > > primaries or the matrix here then. > > Colour spec is described in a different document that I don't have access to. > But I do know the following: > > sRGB 0x01 > Rec-601 0x02 > Rec-709 0x03 > CIE-LUV 0x04 > CIE-XYZ 0x05 > Rec-2020 0x06 > SMPTE-2084 0x07 Ok, you need to research then a bit if this is the whole colorimetry, the primaries, the matrix or a combination of them :) Maybe check another implementation of the JPEG2000 in MPEG-TS spec also?
So, we see that Fic specifies the number of fields in the access unit (1 or 2). Fio specifies the order of the fields: 0 field coding unknown; 1 field with the topmost line is stored first in the access unit; fields are in temporal order; 6 field with the topmost line is stored second in the access unit; fields are in temporal order. Not going to worry about that now, as this is for interlaced. For color spec: 0x00 Unspecified 0x01 IEC 61966-2-1:1999 0x02 Rec. ITU-R BT.601-6 0x03 Rec. ITU-R BT.709-5 0x04 See Tables M.3, M.4 and M.5 0x05 ISO 26428-1 (X'Y'Z') Primaries Values : CIE 1931 XYZ Tri-stimulus values (as defined in ISO 11664-1)
(In reply to Sebastian Dröge (slomo) from comment #127) > (In reply to Aaron Boxer from comment #124) > > > > (In reply to Aaron Boxer from comment #120) > > > > > > > > Is a fixed framerate required, or can it be 0/1 or 0/0? > > > > From what I can see, frame rate is fixed, so 0/0 is not allowed. Also, > > frame rate of 0 is not allowed according to spec, so 0/1 also not allowed. > > So you need to require a framerate in the caps, and it must be higher than > 0/1. And this also makes it required that the input is actually fixed > framerate. > > Seems like a weird requirement for MPEG-TS... are you sure about that? Or > maybe the framerate is only an indication, not necessarily to be correct? No, not sure. Since timecode box is mandatory, doesn't this rule out variable frame rate? > > > > > Time code box is mandatory. So, pretty sure that framerate is fixed. > > From spec, range for time code is: > > > > HH (0-23) > > MM (0-59) > > SS (0-59) > > FF (1-60) > > Ok, so you must invent your own timecode with the API if there is no meta > with an actual timecode on the stream. Or you error out directly if there is > no timecode meta, and require the user to use the timecodestamper element. I > think I prefer requiring timecodestamper. > > You also require videorate to be used for a fixed framerate. > Alright, thanks. > > > What does the spec say exactly for these values? You probably want color > > > primaries or the matrix here then. > > > > Colour spec is described in a different document that I don't have access to. > > But I do know the following: > > > > sRGB 0x01 > > Rec-601 0x02 > > Rec-709 0x03 > > CIE-LUV 0x04 > > CIE-XYZ 0x05 > > Rec-2020 0x06 > > SMPTE-2084 0x07 > > Ok, you need to research then a bit if this is the whole colorimetry, the > primaries, the matrix or a combination of them :) > > Maybe check another implementation of the JPEG2000 in MPEG-TS spec also? Thanks, will do.
(In reply to Aaron Boxer from comment #129) > (In reply to Sebastian Dröge (slomo) from comment #127) > > (In reply to Aaron Boxer from comment #124) > > > > > > (In reply to Aaron Boxer from comment #120) > > > > > > > > > > Is a fixed framerate required, or can it be 0/1 or 0/0? > > > > > > From what I can see, frame rate is fixed, so 0/0 is not allowed. Also, > > > frame rate of 0 is not allowed according to spec, so 0/1 also not allowed. > > > > So you need to require a framerate in the caps, and it must be higher than > > 0/1. And this also makes it required that the input is actually fixed > > framerate. > > > > Seems like a weird requirement for MPEG-TS... are you sure about that? Or > > maybe the framerate is only an indication, not necessarily to be correct? > > No, not sure. Since timecode box is mandatory, doesn't this rule out > variable frame rate? Question is if the timecode box could contain all-zeroes for example. But yes, if actual timecodes are required... This seems very unlike MPEG-TS though, fixed-framerate that is.
(In reply to Sebastian Dröge (slomo) from comment #130) > (In reply to Aaron Boxer from comment #129) > > (In reply to Sebastian Dröge (slomo) from comment #127) > > > (In reply to Aaron Boxer from comment #124) > > > > > > > > (In reply to Aaron Boxer from comment #120) > > > > > > > > > > > > Is a fixed framerate required, or can it be 0/1 or 0/0? > > > > > > > > From what I can see, frame rate is fixed, so 0/0 is not allowed. Also, > > > > frame rate of 0 is not allowed according to spec, so 0/1 also not allowed. > > > > > > So you need to require a framerate in the caps, and it must be higher than > > > 0/1. And this also makes it required that the input is actually fixed > > > framerate. > > > > > > Seems like a weird requirement for MPEG-TS... are you sure about that? Or > > > maybe the framerate is only an indication, not necessarily to be correct? > > > > No, not sure. Since timecode box is mandatory, doesn't this rule out > > variable frame rate? > > Question is if the timecode box could contain all-zeroes for example. But > yes, if actual timecodes are required... > > This seems very unlike MPEG-TS though, fixed-framerate that is. From the spec for time code: ------------------------------------------------------------------------------ Time code box (required) This parameter is a 4-byte unsigned integer field which specifies the Hour (HH: 0-23), Minutes (MM: 0-59), Seconds (SS: 0-59) and Frame Count (FF: 1-60). The HH, MM, SS, and FF fields are individual bytes packed contiguously ----------------------------------------------------------------------------- So, it looks like this box cannot be all zeros.
> > For color spec: > > > 0x00 Unspecified > 0x01 IEC 61966-2-1:1999 > 0x02 Rec. ITU-R BT.601-6 > 0x03 Rec. ITU-R BT.709-5 > 0x04 See Tables M.3, M.4 and M.5 > 0x05 ISO 26428-1 (X'Y'Z') > > Primaries Values : CIE 1931 XYZ Tri-stimulus values (as defined in ISO > 11664-1) Correction: primaries listed above only apply to color spec 0x04 (CIE-LUV)
(In reply to Sebastian Dröge (slomo) from comment #123) > (In reply to Aaron Boxer from comment #122) > > > 1) When the j2k codestream exceed the 65Kb, the buffer after adding the ELSM > > header is later split into two PES packets instead of one which produces > > invalid Mpeg-TS stream. > > Why is it wrong to split the packets? What should be done instead according > to the spec? Or should they be combined differently again? Thanks. According to spec: Each J2K video access unit shall include a PES header with PTS and each PES packet shall contain exactly one J2K video access unit. Each J2K access unit shall contain an elementary stream header (elsm) defined in Table S.1 (as specified in Rec. ITU-T T.800 (2002)/Amd.3 | ISO/IEC 15444-1:2004/Amd.3) followed by one or two codestream(s). In other words, each PES packet must contain exactly one ELSM stream header, and one or two code streams. > > > 2) tsmux doesn't support null packets, which is needed for constant bitrate > > streams > > That's a known limitation of tsmux, and for constant bitrate streams more > than just that is needed
So, it is pretty clear that the packets can't be split.
The size of a PES packet is a 16 bit integer, or not? How can they not be split then?
For video packets you can specify size 0/unbounded IIRC.
Thanks, guys. For this to work, we would really need 32 bit size for PES packets.
Side note: the broadcast industry is currently on the verge of a revolution in how its infrastructure operates : they are moving from expensive, bulky point-to-point SDI interface using custom hardware to packet-switched video-over-IP using consumer off the shelf hardwarer. They are also increasingly using J2K inside the broadcast plant, and J2K over MPEG TS is one of the open solutions being promoted (among a number of other proprietary ones) for transport. Once this GStreamer feature is in place, I think that the framework could play a key role in the new IP infrastructure - that would be very cool.
I think the RTP based solutions here (e.g. SMPTE 2022/2110) seem more promising. MPEG-TS is not really a nice protocol for IP streaming, especially high-bandwidth streaming or even worse over UDP. So for the PES packets here it would make sense to make the 0-sized/unbounded I guess, and add that feature to the muxer (for video at least) so that it doesn't split packets that are bigger than that.
The muxer should be doing that already. It's not like frames larger than 64kB aren't common with other video codecs too. (Or maybe I misunderstood)
(In reply to Sebastian Dröge (slomo) from comment #139) > I think the RTP based solutions here (e.g. SMPTE 2022/2110) seem more > promising. MPEG-TS is not really a nice protocol for IP streaming, > especially high-bandwidth streaming or even worse over UDP. > > That is interesting. Reading the VSF document a little closer http://www.videoservicesforum.org/activity_groups/VSF_TR-01_2013-04-15.pdf it seems that the recommendation is to multiplex the video as an MPEG-TS stream, but actually send it over RTP. The encoded JPEG 2000 image is multiplexed into an MPEG 2 Transport Stream (TS) together with the associated audio and ancillary data. The system supports transparent pass through of linear PCM and non PCM audio, and transparent pass through of ancillary data. The TS is encapsulated in an RTP stream and transmitted over IP to a receiving device (―receiver‖). The receiver will de encapsulate the RTP/IP stream, demultiplex the TS, perform JPEG 2000 decoding, and place the video together with associated audio and ancillary data into the output SDI signal. So, how would that work in GStreamer ?
Would this work ? gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! mpegtsmux ! rtpmp2tpay ! rtpmp2tdepay | tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink
Here is another interesting link http://aimsalliance.org/wp-content/uploads/2017/04/170419-AIMS-White-Paper-Open-IP-Standards.pdf
Yes something like that would work (well, you obviously want a network in the middle, and some kind of RTP jitterbuffer). MPEG-TS over RTP only really makes sense for integration with legacy systems though, when you need to be able to keep a specific MPEG-TS stream as-is. MPEG-TS over RTP otherwise is just duplicating functionality at two different layers and increasing complexity. If you take at e.g. VSF TR4, that one is skipping the MPEG-TS layer For the too big packets, what Tim said makes sense. That should all be there already, so you'll have to understand why it isn't used for JPEG2000. Check what the h264 code paths for example are doing, maybe a flag has to be set somewhere to allow 0-sized PES packets.
(In reply to Sebastian Dröge (slomo) from comment #144) > Yes something like that would work (well, you obviously want a network in > the middle, and some kind of RTP jitterbuffer). MPEG-TS over RTP only really > makes sense for integration with legacy systems though, when you need to be > able to keep a specific MPEG-TS stream as-is. MPEG-TS over RTP otherwise is > just duplicating functionality at two different layers and increasing > complexity. > > If you take at e.g. VSF TR4, that one is skipping the MPEG-TS layer > > > For the too big packets, what Tim said makes sense. That should all be there > already, so you'll have to understand why it isn't used for JPEG2000. Check > what the h264 code paths for example are doing, maybe a flag has to be set > somewhere to allow 0-sized PES packets. Sounds good, thanks!
Currently using the following pipeline to investigate 65536 limit on frame size GST_DEBUG=5 gst-launch-1.0 -v videotestsrc ! video/x-raw,width=640,height=540 ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink 2>&1 | grep "FOO\|BAR"
So, I found that I could get PES packets of size greater than 0xffff if I commented out the following lines in tsmux_stream_initialize_pes_packet and force cur_pes_payload_size equal to zero. if (stream->is_video_stream) { //guint8 hdr_len; //hdr_len = tsmux_stream_pes_header_length (stream); /* Unbounded for video streams if pes packet length is over 16 bit */ //if ((stream->cur_pes_payload_size + hdr_len - 6) > G_MAXUINT16) stream->cur_pes_payload_size = 0; } This code should have been triggered anyways, without my changes, so it looks to me like a bug with the code that calculates cur_pes_payload_size ?. The pipeline is gst-launch-1.0 -v videotestsrc ! video/x-raw,width=1000,height=1000 ! openjpegenc ! jpeg2000parse ! mpegtsmux ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink with my patch
Created attachment 356080 [details] [review] Add jpeg 2000 support for TS mux/demux\ A few more improvements.
Yes, bug in tsmux. Fixing that one, then merging your patch. Thanks!
commit 489466e2a5f919de9064fbd783d038617bb49eac (HEAD -> master) Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jul 21 09:33:54 2017 +0300 tsmux: Add mpegtsmux_jpeg2000.c to meson.build commit 3fe65ad854ff12be9fc63e0cfa7431959be85ee5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jul 21 09:27:20 2017 +0300 tsmux: Store PES payload size in a 32 bit integer While the size in the packet is only 16 bits, we need to handle bigger sizes without overflowing. For video streams this can happen, 0 is written to the stream instead. This fixes muxing of buffers >= 2**16. commit bbbdc2cd7ed282edabb6715a0b9b72d23676ee0f Author: Aaron Boxer <boxerab@gmail.com> Date: Wed Jul 19 10:14:21 2017 -0400 tsmux/tsdemux: Add support for JPEG2000 Based on patches by Milos Seleceni. https://bugzilla.gnome.org/show_bug.cgi?id=753323
This now works, but please implement the remaining pieces in separate patches in another bug (proper timecode usage and only support non-0/1 framerate). I missed that you didn't implement that yet, I hope all the others review comments were covered though :)
(In reply to Sebastian Dröge (slomo) from comment #151) > This now works, but please implement the remaining pieces in separate > patches in another bug (proper timecode usage and only support non-0/1 > framerate). I missed that you didn't implement that yet, I hope all the > others review comments were covered though :) Great, thanks! I will go through this issue, pull out what is left to do and put in another bug.
By the way, the following pipeline: gst-launch-1.0 -v videotestsrc ! openjpegenc ! jpeg2000parse ! mpegtsmux ! rtpmp2tpay ! rtpmp2tdepay ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink is working. So, it looks like we have an implementation of VSF TR01 : encapsulating j2k in mpeg2 TS and sending over RTP. Sweet!
VLC has just implemented a muxer for J2K over MPEG TS, so it should be possible to do interoperability testing between the two projects. https://trac.videolan.org/vlc/ticket/16981 https://git.videolan.org/?p=vlc.git;a=commit;h=e89ca61485d0c28db7d8f3be74c31c9ab304def3
Continuing this saga, I have got my hands on a real live TR1 sample capture file from a Nevion VS902 box! http://s3.amazonaws.com/fox_misc/aperi_tr01_1s_pkts-C.cap.gz What sort of pipeline would I use to read in an mpegts capture file and stream it? Also, I think this may be an interlaced source, so will be interesting what happens when we try to read it.
So, I tried the following pipeline gst-launch-1.0 -v multifilesrc location=~/test.cap ! rtpmp2tdepay ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink with this capture file, but I get this error 0:00:00.077926174 77553 0x55b2399996d0 ERROR rtpbasedepayload gstrtpbasedepayload.c:583:gst_rtp_base_depayload_handle_event:<rtpmp2tdepay0> Segment with non-TIME format not supported
I guess rtpmp2tdepay shouldn't be there..... So, with this pipeline gst-launch-1.0 -v multifilesrc location=~/test.cap ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink Nothing happens after the Pipeline is PREROLLING.... message
The clip contains only one second of video (no audio or ancillary data)
It's pcap so you need to use pcapparse. Wireshark can open it. pcapparse can't handle it for whatever reason (we only support 1st gen pcap, you can re-export in wireshark but even then it does not work). Wireshark can decode the packets as MPEG-TS, so it seems like it's not actually RTP. Trying to decode the packets as RTP in wireshark does not give useful results.
Ah, probably because it's not plain Ethernet/IP but some kind of Virtual LAN (802.1Q) wrapping between Ethernet and IP. That is probably easy to add to pcapparse... if you want to take a look :)
:) This is a little over my head.
Could try this, I guess: https://wiki.wireshark.org/mpeg_dump.lua
What make you say it is pcap? I tried with FFMpeg and it does recognize the mpeg ts stream, but can't parse the j2k.
I guess FFMpeg is also parsing the IP packets
Open it in wireshark :)
:) Yes, indeed it is. I will take a look at pcapparse.
So, the magic number for this cap file is 0xa1b23c4d indicating no endian swapping, but timestamps in nanosecond resolution. I added support for this magic number in pcapparse and ran pipeline under GST_DEBUG=3 GST_DEBUG=3 gst-launch-1.0 -v multifilesrc location=~/test.cap ! pcapparse ! tsdemux ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink Errors: ERROR: from element /GstPipeline:pipeline0/GstMultiFileSrc:multifilesrc0: Error while reading from file "/home/aaron/test.cap". Additional debug info: gstmultifilesrc.c(474): gst_multi_file_src_create (): /GstPipeline:pipeline0/GstMultiFileSrc:multifilesrc0: Could not allocate 67909865 bytes to read file "/home/aaron/test.cap"
Can you put your patch for that somewhere and then we take a look? Another ticket maybe? :)