Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#24 Add Android extensions

open-seeking-sponsorship
nobody
None
5
2014-06-03
2011-09-02
Thomas Gall
No

To libjpeg the Android project added several features. Most in the form of this patch have been ported to libjpeg-turbo. These include:

  • Support for Android build system which does not use the gnu autotools
  • Support for new JCS_RGBA_8888, / red/green/blue/alpha /
  • Support for new JCS_RGB_565 / red/green/blue in 565 format /
  • Support for tile based decode
  • Support for huffman decoding
  • New Decompression APIs including:
    jpeg_start_tile_decompress, seek_input_data, jpeg_read_scanlines_from, jpeg_read_tile_scanline, jpeg_init_read_tile_scanline
    jpeg_build_huffman_index, jpeg_configure_huffman_decoder, jpeg_get_huffman_decoder_configuration, jpeg_create_huffman_index, jpeg_configure_huffman_index_scan, jpeg_destroy_huffman_index
  • new internal APIs include
    consume_input_build_huffman_index, consume_markers, consume_data_build_huffman_index, get_sos_marker_position, decode_mcu_discard_coef,
    configure_huffman_decoder, get_huffman_decoder_configuration, jinit_huff_decoder_no_data, jpeg_decompress_per_scan_setup
    jmin, jset_input_stream_position, jset_input_stream_position_bit, jget_input_stream_position
  • All additions are bounded by #ifdef ANDROID

Errors, typos, etc are mine. While I am not the original author of the Android extensions, I have captured the code and refactored it in a way that I suspect will be more acceptable for inclusion in libjpeg-turbo than it's original form.

Copyright and license for the Android extensions as originally written is as follows:
Copyright (C) 2010 The Android Open Source Project

* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at

*
http://www.apache.org/licenses/LICENSE-2.0

* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.

As the author of the rework, Copyright 2011 Linaro Limited
License same (unless there's preference for something else)

The code has been functionally tested on Linaro's Android Panda LEB and CyanogenMod 7 for Nook Color.

ANDROID.txt | 30 +++
Android.mk | 258 ++++++++++++++++++++++++++
android/config.h | 131 +++++++++++++
android/jconfig.h | 62 ++++++
jccolor.c | 69 +++++++
jdapimin.c | 9
jdapistd.c | 129 ++++++++++++-
jdcoefct.c | 379 ++++++++++++++++++++++++++++++++++++--
jdcolor.c | 532 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
jdhuff.c | 265 ++++++++++++++++++++++++++
jdhuff.h | 1
jdinput.c | 55 +++++
jdmarker.c | 52 +++++
jdmaster.c | 25 ++
jdmerge.c | 357 ++++++++++++++++++++++++++++++++++++
jdphuff.c | 127 ++++++++++++
jdtrans.c | 131 +++++++++++++
jmorecfg.h | 39 +++
jpegint.h | 59 +++++
jpeglib.h | 109 ++++++++++-
jutils.c | 5

Related

Patches: #24

Discussion

1 2 3 .. 7 > >> (Page 1 of 7)
  • Thomas Gall
    Thomas Gall
    2011-09-02

    Initial patch.

     
    Attachments
  • DRC
    DRC
    2011-09-04

    I'll comment on these individually:

    (1) The build system. Is this the only way? It is a maintenance nightmare not only to have yet another build system (we already have two: CMake for Windows and autotools for everyone else) but a build system that is hard-coded -- even to the point that the build number, which is based on the current date and time, is not dynamically-generated. In order for this to be acceptable upstream, there needs to be some way to generate Android.mk dynamically. I think it's easy enough to generate that file using configure, from the same Makefile.am that is used to generate the other Makefiles. Note that you don't have to have autotools to run configure, so this approach would simply require that anyone building libjpeg-turbo on Android first run configure to generate the Android.mk file. If that's not acceptable on your end, then the only other choice is to maintain a hard-coded Android.mk downstream, because I can't maintain it here.

    (2) Null convert: Is this really a common enough case to justify such cleverness? I understand generally what it's doing and why it should be faster for the specific case that it covers, but it seems like that case would be exceedingly rare, so I want to understand why someone went to that amount of trouble to optimize it. There may be a cleaner way of optimizing that function that benefits all platforms, not just Android or ARM.

    (3) Tiled decompression: This is potentially something that could benefit all platforms, but it would need to be implemented in a cleaner manner, by extending existing code rather than introducing a bunch of new routines that are more specific versions of existing routines. The way it is currently implemented is a maintenance nightmare, and it prevents the tiled decompression code from taking advantage of the Huffman optimizations in libjpeg-turbo.

    (4) This:

    +#ifndef ANDROID
    #ifdef __cplusplus
    #ifndef DONT_USE_EXTERN_C
    extern "C" {
    #endif
    #endif
    +#endif

    is nasty. Why not just define DONT_USE_EXTERN_C?

    (5) I think that JCS_RGBA_8888 is redundant and that we can accomplish the same thing by modifying the existing color conversion routines to set the alpha channel to 0xFF, then mapping JCS_RGBA_8888 internally to JCS_EXT_RGBX. That will also give you SIMD acceleration for JCS_RGBA_8888, which it currently lacks.

    (6) JCS_RGB_565 and JCS_RGBA_8888 should be ahead of JCS_EXT_* in the enum. Otherwise, you will break ABI compatibility with existing Android programs that are used to those constants having a particular value.

    (7) The 5-6-5 stuff seems generally OK for inclusion as a non-Android-specific feature, but I would need to do some additional testing and other work, including adding some 5-6-5 images to the unit tests.

    The 5-6-5 stuff is acceptable as-is, and I think it would be an easy fix to implement the 8-8-8-8 stuff using existing code. As far as the rest, I'm perfectly willing to look at re-working it, but I would need funding for that project. I can understand why the Android extensions were implemented as they were-- because it was desirable to keep them from interfering with existing libjpeg code, in case they needed to merge upstream changes. However, the goals for merging a new feature back upstream are somewhat counter to this. I need the new features to, wherever possible, build upon rather than duplicate existing code so that if I modify the code later, I don't have to modify it in two places. By adding a feature to LJT, I am accepting future responsibility for it, which means that it has to be something that I feel I can reasonably maintain.

    Anyway, please reply to the specific questions/comments posed above, and we'll go from there. Maybe we can at least get some of these merged in.

     
  • Thomas Gall
    Thomas Gall
    2011-09-05

    1) I'll continue to look into what we can do that would be acceptable. I agree what is here is less than great.

    2) Ok. I'll follow up.(Labor day here as well)

    3) Ok. Sounds good and happy to take that on.

    4) Will do. And in general I'm to evolve all the android ifdefs down to just one.

    5,6,7 ) Sounds good. I don't mind doing the rework, that's one of the reasons why I'm here. Ultimately I like to see the Android project move over to ltj, ergo the effort to get this going.

    Let's reject this patch and then bring things in in the recommended pieces you've suggested which I'll resubmit.

     
  • (5) I think that JCS_RGBA_8888 is redundant and that we can accomplish
    the same thing by modifying the existing color conversion routines to set
    the alpha channel to 0xFF, then mapping JCS_RGBA_8888 internally to
    JCS_EXT_RGBX. That will also give you SIMD acceleration
    for JCS_RGBA_8888, which it currently lacks.

    ARM NEON code already sets the X channel to 0xFF. For MMX/SSE2 code, this seems to be controlled by RGBX_FILLER_0XFF define (which is apparently off by default). Using 0xFF as a filler value makes RGBX decoded images also perfectly usable as RGBA (0xFF in alpha channel == opaque image), which would be a better default than 0x00 in my opinion.

     
  • (6) JCS_RGB_565 and JCS_RGBA_8888 should be ahead
    of JCS_EXT_* in the enum. Otherwise, you will break ABI
    compatibility with existing Android programs that are
    used to those constants having a particular value.

    As I see it, there are two things to take care of:
    1. Implement the needed functionality in libjpeg-turbo, which may be useful by itself just on any platform.
    2. Be able to emulate the ABI of all the relevant libjpeg variants (libjpeg6 / libjpeg8 / android /...). This is a bit ugly, but happens to be the unfortunate consequence of fragmentation.

    BTW, I have no affiliation with Intel, but wonder if they might be interested in libjpeg-turbo for their x86 Android port?
    http://www.theregister.co.uk/2011/04/20/otellini_on_tablets_andsmartphones/
    Maybe you can try to get some sponsorship there for your work.

     
  • DRC
    DRC
    2011-09-09

    I have changed the behavior of the decompressor so that it now fills the unused byte with 0xFF in all cases, and more importantly, I modified the unit tests to check for this behavior. Thus, it should now be trivial to implement JCS_RGBA_8888 (it will simply map to JCS_EXT_RGBX.)

    As far as RGB 565, I'm OK with implementing that as submitted, but I need some way of validating the 565 buffers so the functionality can be added to 'make test'. Is there a "standard" image format that uses RGB 565, something that I could open in an external program to visually verify?

     
  • Is there a "standard" image format that uses RGB 565,
    something that I could open in an external program to visually verify?

    BMP supports RGB565, and at least gimp can save images in this format.

     
  • I have changed the behavior of the decompressor so that it now
    fills the unused byte with 0xFF in all cases, and more importantly,
    I modified the unit tests to check for this behavior.

    Thanks, that's great. This should make everything easier for supporting JCS extensions in firefox via https://bugzilla.mozilla.org/show_bug.cgi?id=584652 instead of their C and SSE2 code, which currently also sets the unused alpha channel to 0xFF: http://mxr.mozilla.org/mozilla2.0/source/modules/libpr0n/decoders/nsJPEGDecoder.cpp#1137

    But I'm a bit worried about the following part in your commit:

    • / Initialize 4-byte pixels so the alpha channel will be opaque /
      +#if RGB_PIXELSIZE == 4
    • (unsigned int )outptr = 0xFFFFFFFF;
      +#endif
      / Range-limiting is essential due to noise introduced by DCT losses. /
      outptr[RGB_RED] = range_limit[y + Crrtab[cr]];

    In the case if outptr is not 32-bit aligned (is it allowed by libjpeg-turbo API?), this introduces unaligned write which is undefined behaviour in C and is not supported for some architectures (even though it works fine on x86 and modern arm). Moreover, there might be a risk of strict aliasing violation here even though gcc does not seem to issue any warnings in this particular case:
    http://labs.qt.nokia.com/2011/06/10/type-punning-and-strict-aliasing/

     
  • Moreover, there might be a risk of strict aliasing violation
    here even though gcc does not seem to issue any
    warnings in this particular case

    Sorry I missed the part where it was explained that aliasing to 'char' type is explicitly allowed (noting to myself that it makes sense to actually read the page before providing a link to it :) ). So aliasing should not be a problem in this case. And if 4-byte per pixel RGBX variants all require 32-bit alignment for pixel data (which makes a lot of sense) then it is not an issue at all.

     
1 2 3 .. 7 > >> (Page 1 of 7)