Comment on attachment 471713
Patch v1.3.1
I did not review the simd code as that's more work and lower risk
because it's mostly just optimized versions of existing functions. I did
go through the rest of the stuff, hopefully I was thorough enough (there
was a lot of change).
A few of these comments are on Mozilla changes, the others are on the
upstream code. I don't consider it critical that the upstream stuff be
addressed before landing.
>diff --git a/jpeg/MOZCHANGES b/jpeg/MOZCHANGES
>--- a/jpeg/MOZCHANGES
>+++ b/jpeg/MOZCHANGES
>@@ -1,10 +1,42 @@
>+ jchuff.c, jdhuff.c, jdhuff.h
>+ since the versions of these files in libjpeg-turbo are also under the
>+ wxWindows license. (It would have been nicer to revert them to the new
>+ libjepg-8b code, but that doesn't easily integrate with libjepg-turbo.)
s/jepg/jpeg/ (both of them)
>diff --git a/jpeg/jpegint.h b/jpeg/jpegint.h
>--- a/jpeg/jpegint.h
>+++ b/jpeg/jpegint.h
>@@ -364,17 +364,17 @@ EXTERN(void) jinit_color_deconverter JPP
> /* Utility routines in jutils.c */
> EXTERN(long) jdiv_round_up JPP((long a, long b));
>-EXTERN(long) jround_up JPP((long a, long b));
>+EXTERN(size_t) jround_up JPP((size_t a, size_t b));
This change of type here is sort of interesting. It is extended to 64 bits on
Windows and changed to unsigned.
The checkin comment for the change was "Bleepin' Windows uses LLP64, not LP64",
however it seems like the only code that needs the new size is code added by to
jpeg-6b.
(jmemmgr.c and huffman encoder/decoder not included in this patch)
This change removes the type parallelism between jround_up and its sister
function jdiv_round_up,
it's also sort of an abuse of the size_t type because the function is used for
more than just
sizes of objects. Some callers haven't been updated and still do
jround_up((long) val)
I'd suggest using a less general align-to-power-of-two function in jmemmgr.c, I
didn't investigate
why the huffman code needs to round 64bit values
>diff --git a/jpeg/jchuff.c b/jpeg/jchuff.c
>--- a/jpeg/jchuff.c
>+++ b/jpeg/jchuff.c
>@@ -14,16 +14,19 @@
> #include "jchuff.h" /* Declarations shared with jcphuff.c */
>
>+/* MOZILLA CHANGE: libjpeg-turbo doesn't define INLINE in its config file, so
>+ * we define it here. */
>+#define INLINE
This doesn't seem like a very good definition of INLINE, using
the one removed by this patch seems like a better idea.
>diff --git a/jpeg/jcphuff.c b/jpeg/jcphuff.c
>--- a/jpeg/jcphuff.c
>+++ b/jpeg/jcphuff.c
>@@ -218,17 +218,16 @@ dump_buffer (phuff_entropy_ptr entropy)
>
>-INLINE
> LOCAL(void)
> emit_bits (phuff_entropy_ptr entropy, unsigned int code, int size)
> /* Emit some bits, unless we are in gather mode */
>@@ -271,17 +270,16 @@ flush_bits (phuff_entropy_ptr entropy)
>
>-INLINE
> LOCAL(void)
> emit_symbol (phuff_entropy_ptr entropy, int tbl_no, int symbol)
I'm not sure why INLINE needs to go...
>diff --git a/jpeg/jpeglib.h b/jpeg/jpeglib.h
>--- a/jpeg/jpeglib.h
>+++ b/jpeg/jpeglib.h
>@@ -1,58 +1,41 @@
> /*
> * jpeglib.h
> *
> * Copyright (C) 1991-1998, Thomas G. Lane.
>+ * Copyright (C) 2009, D. R. Commander.
> * This file is part of the Independent JPEG Group's software.
> * For conditions of distribution and use, see the accompanying README file.
> *
> * This file defines the application interface for the JPEG library.
> * Most applications using the library need only include this file,
> * and perhaps jerror.h if they want to know the exact error codes.
> */
>
> #ifndef JPEGLIB_H
> #define JPEGLIB_H
>
>-#ifdef XP_OS2
>-/*
>- * On OS/2, the system will have defined RGB_* so we #undef 'em to avoid
>warnings
>- * from jmorecfg.h.
>- */
>-#ifdef RGB_RED
>- #undef RGB_RED
>-#endif
>-#ifdef RGB_GREEN
>- #undef RGB_GREEN
>-#endif
>-#ifdef RGB_BLUE
>- #undef RGB_BLUE
>-#endif
>-
This is probably needed for OS/2 but I'd rather it be fixed upstream.
>diff --git a/jpeg/jcdctmgr.c b/jpeg/jcdctmgr.c
>--- a/jpeg/jcdctmgr.c
>+++ b/jpeg/jcdctmgr.c
> /*
>+ * Find the highest bit in an integer through binary search.
>+ */
>+LOCAL(int)
>+flss (UINT16 val)
>+{
It would be good to use compiler intrinsics like BitScanReverse and
__builtin_clz here when they're available.
>diff --git a/jpeg/jccolor.c b/jpeg/jccolor.c
>--- a/jpeg/jccolor.c
>+++ b/jpeg/jccolor.c
>+ 24, 24, 24, 24, 24, 24, 24, 25, 25, 25, 25, 25, 25, 25, 25, 25,
>+ 26, 26, 26, 26, 26, 26, 26, 26, 26, 27, 27, 27, 27, 27, 27, 27,
>+ 27, 27, 28, 28, 28, 28, 28, 28, 28, 28, 29, 29, 29, 29, 29, 29
>+};
It would be nice to know where these tables come from
>@@ -141,20 +212,20 @@ rgb_ycc_convert (j_compress_ptr cinfo,
>
> while (--num_rows >= 0) {
> inptr = *input_buf++;
> outptr0 = output_buf[0][output_row];
> outptr1 = output_buf[1][output_row];
> outptr2 = output_buf[2][output_row];
> output_row++;
> for (col = 0; col < num_cols; col++) {
>- r = GETJSAMPLE(inptr[RGB_RED]);
>- g = GETJSAMPLE(inptr[RGB_GREEN]);
>- b = GETJSAMPLE(inptr[RGB_BLUE]);
>- inptr += RGB_PIXELSIZE;
>+ r = GETJSAMPLE(inptr[rgb_red[cinfo->in_color_space]]);
>+ g = GETJSAMPLE(inptr[rgb_green[cinfo->in_color_space]]);
>+ b = GETJSAMPLE(inptr[rgb_blue[cinfo->in_color_space]]);
>+ inptr += rgb_pixelsize[cinfo->in_color_space];
If the compiler can't lift the rgb_* out of the loop. It may be worth
doing it by hand.
>@@ -183,36 +254,46 @@ rgb_ycc_convert (j_compress_ptr cinfo,
>- r = GETJSAMPLE(inptr[RGB_RED]);
>- g = GETJSAMPLE(inptr[RGB_GREEN]);
>- b = GETJSAMPLE(inptr[RGB_BLUE]);
>- inptr += RGB_PIXELSIZE;
>+ for (; outptr < maxoutptr; outptr++, inptr += rgbstride) {
> /* Y */
>- outptr[col] = (JSAMPLE)
>- ((ctab[r+R_Y_OFF] + ctab[g+G_Y_OFF] + ctab[b+B_Y_OFF])
>- >> SCALEBITS);
>+ #if BITS_IN_JSAMPLE == 8
>+ *outptr = red_lut[inptr[rindex]] + green_lut[inptr[gindex]]
>+ + blue_lut[inptr[bindex]];
It's not clear what the advantage of this is...
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/612377
Title:
[needs-packaging] libjpeg-turbo
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
