GNOME Bugzilla – Bug 583098
Add jpegparse element
Last modified: 2010-02-01 15:54:53 UTC
As discussed on gst-devel, I'm creating a jpegparse element. It parses a continuous MJPEG stream into single-frame JPEG buffers, much like h264videoparse. It will be based on the depacketization code in jpegdec. Give me a week or so to do this, since I'm tied up with other stuff right now.
Created attachment 135877 [details] [review] Added jpegparse element + test I finally got round to produce the jpegparse element. In the end, it diverges quite a bit from jpegdec. The resync code path is not tested by the element test. Buffer duration is not set - I'm not sure how to do it except based on an externally enforced framerate, and then I don't see if it makes much sense. DISCONT flag is not set. Perhaps it should be set whenever data is discarded, but I'm not convinced of that. Caps are forced to image/jpeg, parsed=(boolean)false/true. If more specific caps are negotiated on a pad they are not passed to the other pad, and if caps are set on an incoming buffer they are not retained. Suggestions on improvements are welcome.
It might make sense to put this into the JPEG plugin. If you don't want that for some reason it would be better to have the parser in gst-plugins-bad for now to give it some further testing, etc :) Apart from that it would be nice if the srcpad caps would include the width/height of the JPEG images, this would address bug #503616 and would be good idea in general.
Please also remove the < 0.10.24 compatibility code. That's not necessary anymore :)
Created attachment 141376 [details] [review] new jpegparser in gst-plugins-bad with header parsing I have taken the freedom to complete this element: - move it to gst-plugins-bad - width and height extraction - format color extraction - if the image is progressive - framerate negotiation - duration calculation Also there's a couple bit for gtk-doc integration. It is applied against master.
Created attachment 141377 [details] [review] remove the jpegparse check (workaround) With the changes, the check is not functional anymore (no header information). As workaround I've another commit to remove it.
It's a pity that the test had to be removed. Anyway, I would like the element to still work even if the jpegs are not 100% OK (e.g. don't contain header information). That way, there's a bigger chance that it also works for JPEG derivatives (like e.g. MxPEG from Mobotix). If the image is not according to standard (or it is somehow corrupted), then gst_jpeg_parse_read_header can do illegal memory reads - it doesn't check position against the buffer size. I also don't see while the number of sections should be limited to 19 - it's allowed to insert as much comments as you like according to the standard! Also, there's a small bug on line : parse->height != parse->height And probably caps should also be updated when fourcc or progressive changes. For the timestamps, I used the first incoming buffer, you now use the last one. In my use case (parsing the jpegs coming from a live http stream) it makes more sense to use the first one - that timestamp already has some network delay, so it's better to not add the additional delay of subsequent network packets. Setting 'packetized' based on the framerate of the sink pad caps is also not a good idea, I think. I realize that jpegdec does it like that, but I don't agree. When you know the framerate of a jpeg stream, you could add with a capsfilter even if the stream isn't parsed. Instead of 'packetized', I'd like to see the 'parsed' property like in e.g. mpeg4videoparse. This enables autoplugging. I realize that that means you can't use this element anymore to extract the detailed caps from an already-parsed stream, but I don't know a solution for that... In fact, is there a reason not to split it into two elements, one for parsing and one for caps?
Thanks for your comments Arnout. (In reply to comment #6) > It's a pity that the test had to be removed. Anyway, I would like the element > to still work even if the jpegs are not 100% OK (e.g. don't contain header > information). That way, there's a bigger chance that it also works for JPEG > derivatives (like e.g. MxPEG from Mobotix). Ahh.. now it makes sense to me :) > Also, there's a small bug on line : > parse->height != parse->height > > And probably caps should also be updated when fourcc or progressive changes. Thanks! > For the timestamps, I used the first incoming buffer, you now use the last one. > In my use case (parsing the jpegs coming from a live http stream) it makes > more sense to use the first one - that timestamp already has some network > delay, so it's better to not add the additional delay of subsequent network > packets. Again, now it makes sense... > Setting 'packetized' based on the framerate of the sink pad caps is also not a > good idea, I think. I realize that jpegdec does it like that, but I don't > agree. When you know the framerate of a jpeg stream, you could add with a > capsfilter even if the stream isn't parsed. > > Instead of 'packetized', I'd like to see the 'parsed' property like in e.g. > mpeg4videoparse. This enables autoplugging. I realize that that means you > can't use this element anymore to extract the detailed caps from an > already-parsed stream, but I don't know a solution for that... Aha... Thanks a lot for your input. Let me rework the code. > In fact, is > there a reason not to split it into two elements, one for parsing and one for > caps? I don't know. I don't get why it should be split. A gstpjpegpacketize and a gstjpegparse ?? Btw, what are your feelings about using gstbaseparse (bug 518857) ??
(In reply to comment #7) > (In reply to comment #6) > > In fact, is > > there a reason not to split it into two elements, one for parsing and one for > > caps? > > I don't know. I don't get why it should be split. A gstpjpegpacketize and a > gstjpegparse ?? Splitting because they have somewhat orthogonal functionality. The packetiser splits a continuous stream into single-frame buffers, the parser analyses caps. The reason to split is because you may need one but not the other. E.g., if the stream comes out of a demuxer or rtp, it is already packetised but perhaps caps are not set correctly yet (e.g. rtpjpegdepay sets w/h but not format - not that I think the format is actually useful). On the other hand, if you just want to packetise and don't care about the metadata (e.g. because you'll just send it to a decoder that doesn't packetise itself), the scanning of the headers is redundant work. In addition, if you set the 'parsed' property on the source/sink caps to enable autoplugging of the parser, then it is no longer possible to create a pipeline like rtpjpegdepay ! jpegparse (assuming rtpjpegdepay also sets the parsed=true property). If jpegpacketize and jpegparse are split up, you could still create a rtpjpegdepay ! jpegparse pipeline (but not the useless rtpjpegdepay ! jpegpacketize pipeline). > Btw, what are your feelings about using gstbaseparse (bug 518857) ?? I'm completely in favour of using base classes, but I didn't realize a parser base class existed when I wrote the element. I was basing myself on mpeg4videoparse which doesn't use the base class (yet). I noticed now that gstbaseparse also assumes packetising and parsing are done together, so I guess my statements here do not reflect the general feeling of the community.
Note that the discussion on packetising and parsing is done in Bug 590014. For now, I think the packetising and parsing in a single element is OK. Demuxers that don't set metadata shouldn't set parsed=true either. That way, autoplugging works and metadata can still be extracted if necessary.
Finally I've found some spare time to continue working on this element. It's not finished yet but the changes can be seen here http://gitorious.org/vjaquez-gstreamer/gst-plugins-bad/commits/jpegparse
Created attachment 147168 [details] [review] new jpegparser in gst-plugins-bad This new element now can pass the checks done by Arnout Vandecappelle; use GstByteReader for the bytestream parsing; a lot of more little changes.
Created attachment 149638 [details] [review] a new revision This new patch fixes an issue with the dri parsing and refactors the sof parsing
I would be interested in having a plugin containing both parser and formatter (muxer). See discussion in bug #486659. the idea is to be able to have different jpeg encoders/decoders, but reuse the APP Segment Parsing and metadata handling. The current jpegparse seems to already has the code to scan for the markers (in gst_jpeg_parse_read_header()) from there we could easily hook up the exif/xmp parsing code (pass the whole segment to a function and get the taglist back). jpegparse coudl also emit the metadata then. Would be nice if it could also handle the COM marker and add that to the metadata (use 'exiv2 -c "test comment" testimage.jpeg' to add a comment. I am a bit unsure how to call the plugin (if it gets a formatter as well) - jpeg is already used by the libjpeg based codec plugin. Could be jpegformat.
Also be careful with #define EXIF 0xe1 this is APP1 and depending on the content seems to be used for - EXIF (then the content is a TIFF file) http://www.digicamsoft.com/exif22/exif22/html/exif22_17.htm - XMP (then the content is XMP metadata, which in turn coudl contain EXIF as RDF XML) http://docs.google.com/viewer?a=v&q=cache:2FWHuN4uTPQJ:xml.coverpages.org/XMP-Embedding.pdf+xmp+jpeg+APP1&hl=de&pid=bl&srcid=ADGEESjhNqZ4SXTtGUy-d3zUVdT_6db3TJmxdDDZJ2R_2ssQvRCwtb_1am1GB_VHfoGKBSx8GQrjjnuVRufU1R13YQsLhcZqaz4QgB_hpJIzupaxfJUcBJv7Yd81rz97LtkuKda-o4PI&sig=AHIEtbTJn4dr5XfdbN7nLFUSNHAzk1qmiQ
Comment on attachment 149638 [details] [review] a new revision Victor, could you please move the plugin to gst-plugin-bad. We start new plugins there.
(In reply to comment #15) > (From update of attachment 149638 [details] [review]) > Victor, could you please move the plugin to gst-plugin-bad. We start new > plugins there. Thanks for your comments! The patch is cooked to be applied in gst-plugins-bad, even when the bug is reported for gst-plugins-good. Though, I can't change the bug component because Arnout is the reporter.
-bad it is :-)
(In reply to comment #17) > -bad it is :-) Thanks Arnout!
(In reply to comment #14) > Also be careful with > #define EXIF 0xe1 > this is APP1 and depending on the content seems to be used for > - EXIF (then the content is a TIFF file) > http://www.digicamsoft.com/exif22/exif22/html/exif22_17.htm > - XMP (then the content is XMP metadata, which in turn coudl contain EXIF as > RDF XML) > > http://docs.google.com/viewer?a=v&q=cache:2FWHuN4uTPQJ:xml.coverpages.org/XMP-Embedding.pdf+xmp+jpeg+APP1&hl=de&pid=bl&srcid=ADGEESjhNqZ4SXTtGUy-d3zUVdT_6db3TJmxdDDZJ2R_2ssQvRCwtb_1am1GB_VHfoGKBSx8GQrjjnuVRufU1R13YQsLhcZqaz4QgB_hpJIzupaxfJUcBJv7Yd81rz97LtkuKda-o4PI&sig=AHIEtbTJn4dr5XfdbN7nLFUSNHAzk1qmiQ Right! I'm cooking a patch changing EXIF for APP1
Created attachment 152482 [details] [review] new revision of jpegparser in gst-plugins-bad This new patch is rebased from the current HEAD of gst-plugins-bad, and also changed the EXIF label to APP1
Changing (In reply to comment #13) > I would be interested in having a plugin containing both parser and formatter > (muxer). See discussion in bug #486659. the idea is to be able to have > different jpeg encoders/decoders, but reuse the APP Segment Parsing and > metadata handling. Got it! Let me find some spare cycles and draft something.
Comment on attachment 152482 [details] [review] new revision of jpegparser in gst-plugins-bad I have changed the plugin name to be "jpegformat" but otherwise committed this as it is.
commit bb5331e2729913aec8ea0f60101049d5b7e590b0 Author: Arnout Vandecappelle <arnout@mind.be> Date: Wed Aug 19 12:22:30 2009 +0200 jpegparse: new jpeg parser element. FIxes #583098 Parse JPEG images, extracts its metadata, set caps and packetize an image per buffer. Lets use new bugs for new patches. Also so that you, Victor, get the attribution for the patches you make.
There is a problem with using it in decodebin: GST_DEBUG="*:2,GST_CAPS*:3,decodebin*:4,jpeg*:4" gst-launch-0.10 -t filesrc location=test.jpeg ! decodebin2 ! fakesink jpegparse gstjpegparse.c:580:gst_jpeg_parse_set_new_caps:<jpegparse0> setting caps on srcpad (hdr_ok=1, have_fps=0) jpegparse gstjpegparse.c:610:gst_jpeg_parse_set_new_caps:<jpegparse0> setting downstream caps on jpegparse0:src to image/jpeg, parsed=(boolean)true, format=(fourcc)I420, interlaced=(boolean)true, width=(int)1152, height=(int)864 decodebin2 gstdecodebin2.c:1852:pad_added_cb:<jpegparse0:src> pad added, chain:0x8953f20 decodebin2 gstdecodebin2.c:1275:analyze_new_pad:<decodebin20> Pad jpegparse0:src caps:image/jpeg, format=(fourcc){ I420, Y41B, UYVY, YV12 }, width=(int)[ 0, 2147483647 ], height=(int)[ 0, 2147483647 ], interlaced=(boolean){ true, false }, framerate=(fraction)[ 0/1, 2147483647/1 ] decodebin2 gstdecodebin2.c:3235:gst_decode_pad_new:<decodebin20> making new decodepad decodebin2 gstdecodebin2.c:1180:gst_decode_bin_autoplug_continue:<decodebin20> autoplug-continue returns TRUE decodebin2 gstdecodebin2.c:1409:analyze_new_pad:<jpegparse0:src> pad has non-fixed caps delay autoplugging decodebin2 gstdecodebin2.c:2574:gst_decode_chain_is_complete:<decodebin20> Chain 0x8953f20 is complete: 0 jpegparse gstjpegparse.c:708:gst_jpeg_parse_chain:<jpegparse0> No further start marker found. basesrc gstbasesrc.c:2458:gst_base_src_loop:<filesrc0> error: Internal data flow error. basesrc gstbasesrc.c:2458:gst_base_src_loop:<filesrc0> error: streaming task paused, reason not-linked (-1) jpegparse gstjpegparse.c:720:gst_jpeg_parse_sink_event:<jpegparse0> event : eos decodebin2 gstdecodebin2.c:1830:pad_event_cb:<decodebin20> Received EOS on a non final pad, this stream ended too early decodebin2 gstdecodebin2.c:2574:gst_decode_chain_is_complete:<decodebin20> Chain 0x8953f20 is complete: 1 decodebin2 gstdecodebin2.c:2919:gst_decode_bin_expose:<decodebin20> Exposing currently active chains/groups decodebin2 gstdecodebin2.c:2946:gst_decode_bin_expose:<decodebin20> All streams finished without buffers decodebin2 gstdecodebin2.c:2948:gst_decode_bin_expose:<decodebin20> error: all streams without buffers any ideas? I might need to downgrade the RANK for next -bad release.
okay, works better in git, I happily take feedback on the changes.