Eliminate the check for ANDROID_LARGE_MEMORY in jpeg decoders

Eliminate the check for ANDROID_LARGE_MEMORY in jpeg decoders

BUG=skia:1282
Committed: https://skia.googlesource.com/skia/+/78d79cc5759d5ec6fb48b28b0cc22d396eb69fb5

[msarett, 2015-04-27 18:36:03]

msarett@google.com changed reviewers:
+ djsollen@google.com, scroggo@google.com

[scroggo, 2015-04-27 20:05:58]

IIUC, this will independently (i.e. independent from fixing the bug in
jmemandroid.c) fix skbug.com/1282 (at least for the attached images - maybe an
image which requires an even larger backing store could still have problems...)

This fixes the bug, and makes things no worse. That said, I'm not sure this is
the right behavior generally (maybe commit this with a FIXME?). Looking back
through the git history, this function was originally added here:

https://skia.googlesource.com/skia/+/12d1625e27e2150fa78799567ddfa4602d229b5f

In that CL, we look at some system property to see how much memory to set the
limit to. Otherwise, we leave it set to the default.

Later on, we decided to just arbitrarily set it to either 30MB or 5MB, depending
on whether the macro  is set:

https://skia.googlesource.com/skia/+/2295c6332512833760060d803cf6ad19a28adc51

I do not know who was setting that originally, but I am guessing it was specific
to devices/configurations. Now it is set if we are not on an arm device, or if
we are and have a VFP (see the Android makefile here:
https://cs.corp.google.com/#android/external/skia/Android.mk&q=ANDROID_LARGE_...)

Regardless, I'm not sure how we settled on these values, and using 30MB on
Android/default elsewhere may or may not be the right decision, but I do think
it's fine for now.

Please update the makefile generator as well. In

https://skia.googlesource.com/skia/+/master/platform_tools/android/gyp_gen/ma...

we no longer need the lines regarding ANDROID_LARGE_MEMORY_DEVICE. You'll also
need to run

./platform_tools/android/tests/makefile_writer_tests.py --rebaseline

to update the expectation.

https://codereview.chromium.org/1103313002/diff/1/src/codec/SkJpegDecoderMgr.cpp
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1103313002/diff/1/src/codec/SkJpegDecoderMgr....
src/codec/SkJpegDecoderMgr.cpp:36: dinfo->mem->max_memory_to_use = 30 * 1024 *
1024;
Why do we use this value? I think I saw in your notes that libjpeg's default is
10MB. Do we know better than libjpeg? (We might - I would believe that was an
old value that does not mean anything today.)

As written, we'll use a different number (the default, which I think is lower)
for non-Android platforms. Any reason for Android to be different?

[scroggo, 2015-04-27 20:24:31]

https://codereview.chromium.org/1103313002/diff/1/src/codec/SkJpegDecoderMgr.cpp
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1103313002/diff/1/src/codec/SkJpegDecoderMgr....
src/codec/SkJpegDecoderMgr.cpp:36: dinfo->mem->max_memory_to_use = 30 * 1024 *
1024;
On 2015/04/27 20:05:58, scroggo wrote:
> Why do we use this value? I think I saw in your notes that libjpeg's default
is
> 10MB. Do we know better than libjpeg? (We might - I would believe that was an
> old value that does not mean anything today.)
> 
> As written, we'll use a different number (the default, which I think is lower)
> for non-Android platforms. Any reason for Android to be different?

I think I've answered my own question: at least when we're building libjpeg (and
when Chrome builds it), we use jmemnobs.c, which ignores this value.

Please remove this whole function, which no longer does anything.

[msarett, 2015-04-27 20:58:50]

Patchset #2 (id:2) has been deleted

[msarett, 2015-04-27 21:13:58]

The reasoning behind this change was that we should consider all currently
supported Android devices as "large" since they all should have VFP (which is
the criteria for large devices in the makefile).  As a side note, it looks like
the VFP flag is defined properly in all cases.

As you noted, there still has not been any thought put into what actual size we
should use.  We just simply noticed that they are all "large".

Having said that, given that we are pushing towards using jmemnobs.c (which does
not use a memory limit), eliminating this function altogether is the right
choice.

[scroggo, 2015-04-27 21:18:23]

On 2015/04/27 21:13:58, msarett wrote:
> The reasoning behind this change was that we should consider all currently
> supported Android devices as "large" since they all should have VFP (which is
> the criteria for large devices in the makefile).  As a side note, it looks
like
> the VFP flag is defined properly in all cases.
> 
> As you noted, there still has not been any thought put into what actual size
we
> should use.  We just simply noticed that they are all "large".
> 
> Having said that, given that we are pushing towards using jmemnobs.c (which
does
> not use a memory limit), eliminating this function altogether is the right
> choice.

Hmm, let's land the change to switch to jmemnobs first. Maybe we'll get some
pushback on that, in which case your patch set 1 (which leaves the function
there, but it only does something on Android) would be the way to go.

[scroggo, 2015-06-11 15:58:18]

nit: please add BUG=skia:1282 to the description

LGTM

Also, I think we can roll back https://codereview.chromium.org/1108013002. Maybe
that should wait until we can sync to the newer version of libjpeg that always
uses jmemnobs.

[msarett, 2015-06-11 17:07:37]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-06-11 17:07:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1103313002/20001

[commit-bot: I haz the power, 2015-06-11 17:13:08]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/78d79cc5759d5ec6fb48b28b0cc22d396eb69fb5