GNOME Bugzilla – Bug 673925
[API] codecparsers: add JPEG baseline parser
Last modified: 2015-06-21 13:45:59 UTC
Created attachment 211859 [details] [review] codecparsers: add JPEG Baseline parser Hi, Feng Yuan (in Cc) wrote a Baseline JPEG codec parser for gst-plugins-bad. The attached patch applies to current 0.10 branch. Could you please review the API, coding style et al. for inclusion? Thanks, Gwenole.
Created attachment 211860 [details] Another JPEG parser API proposal I have also tried to update the API so that to align with existing codecparsers style like MPEG-2 video parser in particular. Here is another gstjpegparser.h proposal. I have not tried to migrate the existing gstjpegparser.c code to it yet. Please tell us what you prefer. I think we could also find an intermediate version that would simplify porting.
the git repo containing it is here : https://gitorious.org/vaapi/gstreamer-vaapi
> I have also tried to update the API so that to align with existing codecparsers > style like MPEG-2 video parser in particular. Here is another gstjpegparser.h > proposal. I have not tried to migrate the existing gstjpegparser.c code to it > yet. Please tell us what you prefer. I think we could also find an intermediate > version that would simplify porting. Unfortunately the mpeg-2 video parser was not the best example of the kind of API we prefer. Could you have a look at how the API has been refactored recently and update the jpeg parser? Mostly: - no GList * return for _parse() - plain gboolean return values Not sure the GstJpegProfile enum is entirely right, it's a bit unusual to combine enums and flags in the same enumeration, that's going to trip up bindings too. And I am not sure 'profile' is the right term here. But this is already well into bikeshedding territory, the other issues are more important.
Thanks Tim for the review. Nuking away GList was indeed in my plan. For the GstJpegProfile thing, we could stick to the original SOFn notation, thus removing the enum, but it's not quite pretty IMHO. What do you think?
I don't really mind, could be a plain SOF0-15 and/or something like SOF_MARKER_BASELINE SOF_MARKER_EXTENDED_SEQUENTIAL_HUFFMAN SOF_MARKER_PROGRESSIVE_HUFFMAN SOF_MARKER_LOSSLESS_HUFFMAN SOF_MARKER_DIFFERENTIAL_SEQUENTIAL_HUFFMAN SOF_MARKER_DIFFERENTIAL_PROGRESSIVE_HUFFMAN SOF_MARKER_DIFFERENTIAL_LOSSLESS_HUFFMAN SOF_MARKER_EXTENDED_SEQUENTIAL_ARITHMETIC SOF_MARKER_PROGRESSIVE_ARITHMETIC SOF_MARKER_LOSSLESS_ARITHMETIC SOF_MARKER_DIFFERENTIAL_SEQUENTIAL_ARITHMETIC SOF_MARKER_DIFFERENTIAL_PROGRESSIVE_ARITHMETIC SOF_MARKER_DIFFERENTIAL_LOSSLESS_ARITHMETIC or you just save the sof_marker and then provide "getters" for a "profile" enum (baseline,...,lossless) and entropy coding (none, huffman, arithmetic). Do you think you might be able to have a look at this soon? I'm asking because I'd be interested in using it for a new jpeg parser element.
Hi, I have started the work. The type-offset-size struct was converted to a GstJpegMarkerSegment struct, some fixes to bound checks. I kept a guint8 marker code for now.
Hi, I have updated the codecparsers to gst-plugins-bad master: <http://cgit.freedesktop.org/~gb/gst-plugins-bad/> (codecparsers-jpeg branch) No sensible performance improvement. From the very first iteration of the parser to this one, the time spent in jpeg parsing + decode functions on CPU decreased by ~28%. From the previous version based on GList to this one, this is very negligible.
Gwenole: are you still working on this ? Last you told me on IRC that you were still making some changes IIRC.
Gwenole already made some changes for JPEG parser. Currently all code are in git://gitorious.org/vaapi/gstreamer-vaapi.git files: gst-libs/gst/codecparsers/gstjpegparser.h gst-libs/gst/codecparsers/gstjpegparser.c
Hi Tim, I had completed the changes I wanted to make but I think I forgot to update my personal repo for gst-plugins-bad. I will push those when I get to the office. http://cgit.freedesktop.org/~gb/gst-plugins-bad/ (codecparsers-jpeg branch). Otherwise, gst-vaapi contains the latest bits.
Created attachment 224081 [details] [review] codecparsers: add JPEG bitstream parser I have updated my personal repo with the current parser. Also attached here for convenience.
Created attachment 255630 [details] [review] codecparsers: jpeg: fix default Huffman tables generation commit 10a38649d20f45c0c7b9f6a05d8eb14202834fd0 Author: Wind Yuan <feng.yuan@intel.com> Date: Thu Jun 13 13:22:18 2013 +0800 codecparsers: jpeg: fix default Huffman tables generation. Fix build_huffman_table() to correctly fill in the associated HUFFVAL entries to the default Huffman tables.
Created attachment 255631 [details] [review] codecparsers: jpeg: fix calculation of segment size commit 772be25c90832a7c31b528bc9710e0843baeb7e4 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Sep 18 15:56:52 2013 +0200 codecparsers: jpeg: fix calculation of segment size. The size of a marker segment is defined to be exclusive of any initial marker code. So, fix the size for SOI, EOI and APPn segments but also the size of any possible segment that is usually "reserved" or not explicitly defined. https://bugzilla.gnome.org/show_bug.cgi?id=707447
Created attachment 255632 [details] [review] codecparsers: jpeg: fix and optimize scan for next marker code commit d6b1b3a389b0f185cc9eeca89b7cc476e3d780d5 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Sep 24 16:10:45 2013 +0200 codecparsers: jpeg: fix and optimize scan for next marker code. Fix scan for next marker code when there is an odd number of filler (0xff) bytes before the actual marker code. Also optimize the loop to execute with fewer instructions (~10%). This fixes parsing for Spectralfan.mov.
I have attached 3 new patches. I could squash 2 of them (mine) and leave out Feng's patch as is if you prefer. The gst-vaapi decoder was updated to match the current JPEG parser API and implementation. Overall performance was also improved by +15 to +25%. I don't see yet any other better algorithm to scan for the next marker code, while preserving correctness, few instruction counts, branches and memory loads. I am also wondering about s/GstJpegMarkerSegment/GstJpegSegment/ and add in a const guint8 * data similarly to the MPEG-2 packet definition. But that's just cosmetics. I don't mind either way.
Thanks, looks like you have patches for some of the issues I noticed while testing this. I have also renamed GstJpegMarkerSegment locally and changed the API a little to match the mpeg2 API. Should get this in in the 1.3.x cycle.
Hi Tim, (In reply to comment #16) > Thanks, looks like you have patches for some of the issues I noticed while > testing this. > > I have also renamed GstJpegMarkerSegment locally and changed the API a little > to match the mpeg2 API. Should get this in in the 1.3.x cycle. Did you push that somewhere? I probably missed that because I git rebased my codecparsers tree the other day and the only differences that came out were still the jpeg parser (and mvc/svc changes). Thanks.
Ah no, this was work-in-progress that got lost when I migrated to a new laptop, sorry. I will dig it out and push it in time for 1.4.
At last: commit d8963a015f51f872f8a05d50839370b7f52b9dbd Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jun 21 13:55:29 2015 +0100 docs: add new JPEG codecparser API to the docs And sprinkle some more Since markers commit e7512329cac59b60bf911639320c746d637f9f90 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jun 21 11:51:38 2015 +0100 examples: move vp8 parser test to codecparser example directory commit 82be4ad39537c8328dc9d458e4ecbde8c68e3770 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jun 21 11:20:57 2015 +0100 examples: add small jpeg codecparser test https://bugzilla.gnome.org/show_bug.cgi?id=673925 commit 7d8f69450146b6bfe98c71b1a34bea4492b46dce Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat Jun 20 22:49:23 2015 +0100 codecparsers: jpeg: fix validity checking of data parsed g_return_val_if_fail() and g_assert() are not appropriate for checking untrusted external data. https://bugzilla.gnome.org/show_bug.cgi?id=673925 commit 0f04a61bbe2699c7097e01eb552d70bfa7741929 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat Jun 20 19:52:42 2015 +0100 codecparsers: jpeg: fix up API - add data pointer to GstJpegSegment and pass segment to all parsing functions, rename accordingly - shorten GstJpegMarkerCode enum type name to GstJpegMarker - move function gtk-doc blurbs into .c file - add since markers - flesh out docs for SOF markers https://bugzilla.gnome.org/show_bug.cgi?id=673925 commit 4a85958909ff37dcfd28dd7149bec7da2fd652f8 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jun 14 11:41:52 2015 +0100 codecparsers: jpeg: tweak API a little https://bugzilla.gnome.org/show_bug.cgi?id=673925 commit 441478d161f52ed1e20a197a37e96f97c1aa817d Author: Tim-Philipp Müller <tim@centricular.com> Date: Sun Jun 14 19:01:12 2015 +0100 codecparsers: jpeg: hide gst_jpeg_scan_for_marker_code() Make this function private for now, since it's unclear whether it's actually needed seeing that gst_jpeg_parse() scans too. https://bugzilla.gnome.org/show_bug.cgi?id=673925 commit d279e12c8f93cf88e397f40155079fa4439e3ccc Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Sep 24 16:10:45 2013 +0200 codecparsers: jpeg: fix and optimize scan for next marker code. Fix scan for next marker code when there is an odd number of filler (0xff) bytes before the actual marker code. Also optimize the loop to execute with fewer instructions (~10%). This fixes parsing for Spectralfan.mov. commit 41ed5c0266489f7247a8f6a2fc13bb4e0c73c516 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Sep 18 15:56:52 2013 +0200 codecparsers: jpeg: fix calculation of segment size. The size of a marker segment is defined to be exclusive of any initial marker code. So, fix the size for SOI, EOI and APPn segments but also the size of any possible segment that is usually "reserved" or not explicitly defined. https://bugzilla.gnome.org/show_bug.cgi?id=707447 commit c83e413656f9c1e9918cfa00c53234327ac83071 Author: Wind Yuan <feng.yuan@intel.com> Date: Thu Jun 13 13:22:18 2013 +0800 codecparsers: jpeg: fix default Huffman tables generation. Fix build_huffman_table() to correctly fill in the associated HUFFVAL entries to the default Huffman tables. commit 7e7b4d68f4081f6e7e3bf851d4a041179b594ff9 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Wed Sep 12 10:40:06 2012 +0200 codecparsers: jpeg: add JPEG bitstream parser Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> https://bugzilla.gnome.org/show_bug.cgi?id=673925
Comment on attachment 255632 [details] [review] codecparsers: jpeg: fix and optimize scan for next marker code (commit picked from vaapi codecparsers repo)
Comment on attachment 255631 [details] [review] codecparsers: jpeg: fix calculation of segment size (commit picked from vaapi codecparsers repo)
Comment on attachment 255630 [details] [review] codecparsers: jpeg: fix default Huffman tables generation (commit picked from vaapi codecparsers repo)
Comment on attachment 224081 [details] [review] codecparsers: add JPEG bitstream parser (commit picked from vaapi codecparsers repo)