After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673925 - [API] codecparsers: add JPEG baseline parser
[API] codecparsers: add JPEG baseline parser
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-04-11 17:13 UTC by Gwenole Beauchesne
Modified: 2015-06-21 13:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
codecparsers: add JPEG Baseline parser (26.87 KB, patch)
2012-04-11 17:13 UTC, Gwenole Beauchesne
none Details | Review
Another JPEG parser API proposal (12.21 KB, text/plain)
2012-04-11 17:18 UTC, Gwenole Beauchesne
  Details
codecparsers: add JPEG bitstream parser (37.52 KB, patch)
2012-09-12 09:33 UTC, Gwenole Beauchesne
committed Details | Review
codecparsers: jpeg: fix default Huffman tables generation (921 bytes, patch)
2013-09-24 14:37 UTC, Gwenole Beauchesne
committed Details | Review
codecparsers: jpeg: fix calculation of segment size (1.57 KB, patch)
2013-09-24 14:38 UTC, Gwenole Beauchesne
committed Details | Review
codecparsers: jpeg: fix and optimize scan for next marker code (1.41 KB, patch)
2013-09-24 14:38 UTC, Gwenole Beauchesne
committed Details | Review

Description Gwenole Beauchesne 2012-04-11 17:13:22 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.
Comment 1 Gwenole Beauchesne 2012-04-11 17:18:24 UTC
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.
Comment 2 Edward Hervey 2012-06-19 11:40:46 UTC
the git repo containing it is here : https://gitorious.org/vaapi/gstreamer-vaapi
Comment 3 Tim-Philipp Müller 2012-07-10 22:23:05 UTC
> 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.
Comment 4 Gwenole Beauchesne 2012-07-11 04:59:04 UTC
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?
Comment 5 Tim-Philipp Müller 2012-07-11 16:08:07 UTC
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.
Comment 6 Gwenole Beauchesne 2012-07-16 19:12:46 UTC
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.
Comment 7 Gwenole Beauchesne 2012-07-17 14:56:01 UTC
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.
Comment 8 Tim-Philipp Müller 2012-09-11 23:06:17 UTC
Gwenole: are you still working on this ? Last you told me on IRC that you were still making some changes IIRC.
Comment 9 Wind Yuan 2012-09-12 01:41:44 UTC
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
Comment 10 Gwenole Beauchesne 2012-09-12 04:08:12 UTC
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.
Comment 11 Gwenole Beauchesne 2012-09-12 09:33:07 UTC
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.
Comment 12 Gwenole Beauchesne 2013-09-24 14:37:35 UTC
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.
Comment 13 Gwenole Beauchesne 2013-09-24 14:38:13 UTC
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
Comment 14 Gwenole Beauchesne 2013-09-24 14:38:46 UTC
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.
Comment 15 Gwenole Beauchesne 2013-09-24 14:44:19 UTC
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.
Comment 16 Tim-Philipp Müller 2013-09-24 14:47:10 UTC
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.
Comment 17 Gwenole Beauchesne 2014-03-25 10:35:36 UTC
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.
Comment 18 Tim-Philipp Müller 2014-03-25 10:48:20 UTC
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.
Comment 19 Tim-Philipp Müller 2015-06-21 13:43:19 UTC
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 20 Tim-Philipp Müller 2015-06-21 13:45:30 UTC
Comment on attachment 255632 [details] [review]
codecparsers: jpeg: fix and optimize scan for next marker code

(commit picked from vaapi codecparsers repo)
Comment 21 Tim-Philipp Müller 2015-06-21 13:45:40 UTC
Comment on attachment 255631 [details] [review]
codecparsers: jpeg: fix calculation of segment size

(commit picked from vaapi codecparsers repo)
Comment 22 Tim-Philipp Müller 2015-06-21 13:45:49 UTC
Comment on attachment 255630 [details] [review]
codecparsers: jpeg: fix default Huffman tables generation

(commit picked from vaapi codecparsers repo)
Comment 23 Tim-Philipp Müller 2015-06-21 13:45:59 UTC
Comment on attachment 224081 [details] [review]
codecparsers: add JPEG bitstream parser

(commit picked from vaapi codecparsers repo)