Remove image decoder and encoder autoregistration

Remove image decoder and encoder autoregistration

Remove image decoder and encoder autoregistration through SkFactory.
Replace this with normal sequential if -statements and #ifdefs.

The ability to compile a encoder/decoder to the library just by including
a .cpp file was not fulfilled. In addition to adding the file, one would need
to add the new entry points to multiple other places, such as SkForceLinking.
The autoregistration caused the encoder/decoder system to be more laboursome
to maintan than without it.

Simplifies using images library with embedders like Chromium. Avoids following
problems:

 * Linking of decoders do not need to be forced in every executable that
   links to the library

 * Decoder and encoder application order is not undefined

 * Decoder and encoder application order does not need to be encoded in
   build system. The previous order was not as expected on all
   platforms, for example on windows.

Removes the possibility for library client to replace existing
decoders/encoders through the API. The possibility still exists
for clients that "recompile" Skia (such as Chromium).

BUG=skia:2992

[Kimmo Kinnunen, 2014-10-20 12:52:54]

kkinnunen@nvidia.com changed reviewers:
+ scroggo@google.com

[Kimmo Kinnunen, 2014-10-20 12:52:55]

would it be possible to replace FORCE_IMAGE_DECODER_LINKING with plain old
ifdef'ing?

[Kimmo Kinnunen, 2014-10-20 14:03:39]

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_Fa...
File src/images/SkImageEncoder_Factory.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_Fa...
src/images/SkImageEncoder_Factory.cpp:26: #if defined(SK_BUILD_FOR_WIN)
I can double-check the encoder order and fix potential compile problems on
windows, mac and iOS if the approach is acceptable..

[reed1, 2014-10-20 14:39:57]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-20 14:39:58]

There are a lot of separate CLs related to this. Perhaps we should come to some
consensus about the alternate design before commenting separately on the
different Cls.

[scroggo, 2014-10-20 15:34:41]

On 2014/10/20 14:39:58, reed1 wrote:
> There are a lot of separate CLs related to this. Perhaps we should come to
some
> consensus about the alternate design before commenting separately on the
> different Cls.

+1

Kimmo, would you like to set up a VC so we can discuss your goals? The image
decoders are used by both Android and our tests, so we want to make sure we do
the right thing for those clients as well.

[Kimmo Kinnunen, 2014-10-20 16:19:04]

On 2014/10/20 15:34:41, scroggo wrote:
> On 2014/10/20 14:39:58, reed1 wrote:
> > There are a lot of separate CLs related to this. Perhaps we should come to
> some
> > consensus about the alternate design before commenting separately on the
> > different Cls.
> 
> +1
> 
> Kimmo, would you like to set up a VC so we can discuss your goals? The image
> decoders are used by both Android and our tests, so we want to make sure we do
> the right thing for those clients as well.

Sure. Though I can set up a phone conference only tomorrow at the office. 
For today, I'm available now for 3 hours if you could set it up. Hangouts also
works with the address ktkinnun@gmail.com..

[scroggo, 2014-10-22 19:54:31]

On 2014/10/20 16:19:04, Kimmo Kinnunen wrote:
> On 2014/10/20 15:34:41, scroggo wrote:
> > On 2014/10/20 14:39:58, reed1 wrote:
> > > There are a lot of separate CLs related to this. Perhaps we should come to
> > some
> > > consensus about the alternate design before commenting separately on the
> > > different Cls.
> > 
> > +1
> > 
> > Kimmo, would you like to set up a VC so we can discuss your goals? The image
> > decoders are used by both Android and our tests, so we want to make sure we
do
> > the right thing for those clients as well.
> 
> Sure. Though I can set up a phone conference only tomorrow at the office. 
> For today, I'm available now for 3 hours if you could set it up. Hangouts also
> works with the address mailto:ktkinnun@gmail.com..

Hi, sorry for the radio silence. We've been busy with some other things. Thank
you
for writing up the document. I made one comment inline regarding my main
concern/
confusion:

Right now, we build the _empty.cpp files in Chromium to disable features that
Chromium does not want. It looks like your approach uses this method doubly:

Build Skia using Skia's gyp files:
- build SkImageDecoder_*.cpp
- build SkImagesSupport_default.cpp

Build Chromium:
- build SkImageDecoder_empty.cpp
- build SkImagesSupport_noimages.cpp

Build Chromium with Skia tests:
- build SkImageDecoder_empty.cpp
- build some version of SkImagesSupport

Is that correct? Or does a Chromium build just skip SkImagesSupport, leaving
those
functions undefined but never called?

It seems like if your special Chromium build with Skia tests has to use a
different
SkImagesSupport anyway it could use different SkImageDecoder implementations as
well. Or am I missing something?

[Kimmo Kinnunen, 2014-10-23 08:33:52]

On 2014/10/22 19:54:31, scroggo wrote:
> It seems like if your special Chromium build with Skia tests has to use a
> different
> SkImagesSupport anyway it could use different SkImageDecoder implementations
as
> well. Or am I missing something?

Maybe? The intention is that the decoder_empty is removed. As said: one can not
decode images with an empty decoder. Images need to be decoded for this
use-case.

Wrt. headings like "Build Chromium with Skia tests": it is not clear if you
refer to "scenarios with or without this work" or actual build targets. I'm
assuming the latter, since the _noimages is referred to in all the points.

There's no distinct Skia for "Build Chromium" and "Build Chromium with Skia
tests". First you build Skia. Then you build dm. Then you link Skia to dm.
Everything not part of Chromium Skia will be compiled as part of step "build
dm". I think this is explained in the design doc. It would not make much sense
to build yet another Skia and test that, if the intention is to test the
bit-by-bit exact library Chromium uses..

The _empty files exist due to Chromium. Removing them breaks the Chromium build,
so I left that last. I have already 5 patches in flight, thus not posting or
working on the further patches. I wanted to get the positive feedback in order
to continuing the work. I did not receive such, thus I postponed the work. I
don't think anybody would assume these patches would be sufficient to complete
the use-case.

Wrt your peculiarly intense focus on SkImagesSupport: If it was up to me,
SkImagesSupport would not be needed. It will contain the one API implementation
function (due to api using a default parameter that incorrectly crosses skia
internal library bounds) and a debugging function (that incorrectly references
across skia internal library bounds). I'd rather remove both. However, I would
imagine this would be acceptable compared to _empty things. At least
SkImagesSupport highlights the exact violations. Maybe I was wrong..

[Kimmo Kinnunen, 2014-10-23 08:49:04]

On 2014/10/23 08:33:52, Kimmo Kinnunen wrote:
> There's no distinct Skia for "Build Chromium" and "Build Chromium with Skia
> tests". First you build Skia. Then you build dm. Then you link Skia to dm.

Even if we're not talking about the Chromium -side modifications, I want to just
pre-empt anybody hanging to the detail here:
By this I do not mean that the work would end up making Chromium build somehow
build the testing parts. What I try to say here is that the Skia library
Chromium builds is still optimally small (it will be smaller than today), and it
is every way similar to Skia of Chromium today. There would just be additional
target, such as 'ninja dm', which would then build all the files necessary to
run the tests, but use the normal Chromium Skia library that is also used by
other parts of Chromium.

[scroggo, 2014-11-12 18:00:13]

By itself, I think this is a great change! The current registration system is a
bandaid on top of a bandaid (if not more).

Comments in line

https://codereview.chromium.org/670453002/diff/1/gyp/images.gyp
File gyp/images.gyp (left):

https://codereview.chromium.org/670453002/diff/1/gyp/images.gyp#oldcode48
gyp/images.gyp:48: # IMPORTANT: The build order of the SkImageDecoder_*.cpp
files
Generally, we keep these in alphabetical order. Since this deviation is no
longer useful, please alphabetize.

https://codereview.chromium.org/670453002/diff/1/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://codereview.chromium.org/670453002/diff/1/gyp/public_headers.gypi#newc...
gyp/public_headers.gypi:1: # Include this gypi to include all public header
files that exist in the
FYI: This file has been removed.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
File src/images/SkImageDecoder_Factory.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:2: * Copyright 2013 The Android Open
Source Project
2014 Google Inc

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:27: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
line too long.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:46: } gDecoderFactoryFuncs[] = {
Could you add a comment here that the order is meaningful? (Like the one removed
from images.gyp?)

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:59: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
line too long

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:74: FactoryCreateFunc createFunc =
gDecoderFactoryFuncs[i].createFunc;
Here's a thought:

I notice that in every one of the decoder implementation files, we essentially
have the following function:

SkCreate_(stream) {
    if (SkDetectFormat_(stream) != kUnknown) {
        return <my_decoder>
    }
    return null;
}

I think we could share some code if we used an actual factory object:

class SkDecoderFactory {
public:
    virtual Format detectFormat(SkStreamRewindable*) const = 0;
    virtual SkImageDecoder* createMyDecoder() const = 0;
    SkImageDecoder* createIfSupported(SkStreamRewindable* stream) {
        if (this->detectFormat(stream) == kUnknown) {
            return NULL;
        }
        return createMyDecoder();
    }
};

Then just have an array of Factory objects.

for (size_t i = 0; i < SK_ARRAY_COUNT(gFactoryObjs); ++i) {
    SkImageDecoder* codec = gFactoryObjs[i]->createIfSupported(stream);
    // rewind/check success
    if (codec) return codec;
}
return NULL;

What do you think?

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:80: // our image decoder's require that
rewind is supported so we fail early
decoders*

(I know this duplicating an old typo, but as long as we're moving the comment,
please remove the error.)

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:113: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
line too long

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_Fa...
src/images/SkImageDecoder_Factory.cpp:114: movie = SkGIFMovieCreate(stream);
The old code does a rewind if movie is null.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_kt...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:248: return SkImageDecoder::kKTX_Format;
indent 4 spaces

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:254: if (SkDetectFormatKTXImageDecoder(stream)
== SkImageDecoder::kKTX_Format) {
Why not call SkKTXFile::is_ktx?

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:255: return SkNEW(SkKTXImageDecoder);
indent 4 spaces.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_kt...
src/images/SkImageDecoder_ktx.cpp:325: return (SkImageEncoder::kKTX_Type == t) ?
SkNEW(SkKTXImageEncoder) : NULL;
4 spaces.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
File src/images/SkImageDecoder_libbmp.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libbmp.cpp:134: !memcmp(buffer, kBmpMagic,
sizeof(kBmpMagic))) {
I don't think it's in the style guide, but this is harder to read since the
second line of the if statement lines up with the code inside the block.

One thing that would help is to put the brace on the next line. (Or you could
indent this line 8 spaces instead of 4.)

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
File src/images/SkImageDecoder_libico.cpp (left):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libico.cpp:28: 
Why the delete?

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libjpeg.cpp:1374: SkImageDecoder::Format
SkDetectFormatJPEGImageDecoder(SkStreamRewindable* stream) {
nit: The change is easier to follow without moving the function

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libpng.cpp:76: 
Why the extra line?

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libpng.cpp:1102: !png_sig_cmp((png_bytep) buf,
(png_size_t)0, PNG_BYTES_TO_CHECK)) {
nit: move brace to next line.

(Again, easier to follow if you hadn't moved the functions.)

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
File src/images/SkImageDecoder_libpng.h (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_li...
src/images/SkImageDecoder_libpng.h:1: 
nit: no need for this empty line.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_pk...
File src/images/SkImageDecoder_pkm.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_pk...
src/images/SkImageDecoder_pkm.cpp:108: if (etc1_pkm_is_valid(buf)) {
nit: why not &&?

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_wb...
File src/images/SkImageDecoder_wbmp.h (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageDecoder_wb...
src/images/SkImageDecoder_wbmp.h:1: 
No need for this blank line.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_Fa...
File src/images/SkImageEncoder_Factory.cpp (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_Fa...
src/images/SkImageEncoder_Factory.cpp:26: #if defined(SK_BUILD_FOR_WIN)
On 2014/10/20 14:03:39, Kimmo Kinnunen wrote:
> I can double-check the encoder order and fix potential compile problems on
> windows, mac and iOS if the approach is acceptable..

FYI, we have compile bots for windows and mac that will run as part of the
commit queue (or you can manually run them from the reitveld issue). I don't see
one for iOS though.

I think the approach is good. I sort of hate that we need the same ifdef
conditions in two places, but I can't think of a better way.

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_ar...
File src/images/SkImageEncoder_argb.h (right):

https://codereview.chromium.org/670453002/diff/1/src/images/SkImageEncoder_ar...
src/images/SkImageEncoder_argb.h:1: 
no need for this line.

[reed1, 2014-11-12 18:31:46]

I do not like the direction of the change per-se, as I find the current scheme
much simpler, but I will read more and will happily discuss.

[Kimmo Kinnunen, 2014-11-13 14:02:27]

On 2014/11/12 18:31:46, reed1 wrote:
> I do not like the direction of the change per-se, as I find the current scheme
> much simpler, but I will read more and will happily discuss.

I guess from the point of view of adding a new decoder, the current scheme is
simpler.

However, the current scheme is not simpler from the points of view that the
commit msg tries to outline. I think following the git history of the lines
removed here, one can find the debugging/maintenance cost of the current scheme.

I don't know if debugging the linker behavior that led to the current bunch of
solutions was particularly simple..

[Kimmo Kinnunen, 2014-11-13 14:04:49]

On 2014/11/13 14:02:27, Kimmo Kinnunen wrote:
> On 2014/11/12 18:31:46, reed1 wrote:
> > I do not like the direction of the change per-se, as I find the current
scheme
> > much simpler, but I will read more and will happily discuss.
> 
> I guess from the point of view of adding a new decoder, the current scheme is
> simpler.


.. and I'm actually that sure whether even adding new decoder is any more
simple. The ifdef bunch exists in current scheme also (the force linking file).

[Kimmo Kinnunen, 2014-11-18 08:29:46]

https://chromiumcodereview.appspot.com/670453002/diff/1/gyp/images.gyp
File gyp/images.gyp (left):

https://chromiumcodereview.appspot.com/670453002/diff/1/gyp/images.gyp#oldcode48
gyp/images.gyp:48: # IMPORTANT: The build order of the SkImageDecoder_*.cpp
files
On 2014/11/12 18:00:12, scroggo wrote:
> Generally, we keep these in alphabetical order. Since this deviation is no
> longer useful, please alphabetize.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/gyp/public_headers.gypi
File gyp/public_headers.gypi (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/gyp/public_headers.gy...
gyp/public_headers.gypi:1: # Include this gypi to include all public header
files that exist in the
On 2014/11/12 18:00:12, scroggo wrote:
> FYI: This file has been removed.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_Factory.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:2: * Copyright 2013 The Android Open
Source Project
On 2014/11/12 18:00:12, scroggo wrote:
> 2014 Google Inc

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:27: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
On 2014/11/12 18:00:12, scroggo wrote:
> line too long.

Done. Sorry about the whitespace problems, I've fixed my editor since.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:46: } gDecoderFactoryFuncs[] = {
On 2014/11/12 18:00:12, scroggo wrote:
> Could you add a comment here that the order is meaningful? (Like the one
removed
> from images.gyp?)

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:59: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
On 2014/11/12 18:00:12, scroggo wrote:
> line too long

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:74: FactoryCreateFunc createFunc =
gDecoderFactoryFuncs[i].createFunc;
On 2014/11/12 18:00:12, scroggo wrote:
> I think we could share some code if we used an actual factory object:
...
> What do you think?

My opinion would be to not do it.
Considering pros:
- Save 2 statements for every decoder
Considering cons:
- Introduce 1 class
- Introduce problems with mutable state: threading issues, global initialization
issues, writable data (the issues we try to solve with this patch in the first
place)
- Introduce unused and unneeded polymorphism, which makes the code harder to
reason about. (concrete implementations are known by this file, so no need for
polymorphism)
- Unneeded use of a factory causing confusion (Factory object without state is
simply a function mapping args to created instances. See factory object  ==>
expect state ==> notice no state ==> slight confusion.)

To me, having two three-line functions per decoder is satisfyingly simple
approach. I don't feel that strongly about it, so if you want, I can add some
more lines in form of class declarations.

I changed below call to createFunc with following:
if (detectFormatFunc(stream) != kUnknown) {
  codec = createFunc(stream)
}

This way one can remove the if statements from the create functions. Would this
suffice?

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:80: // our image decoder's require that
rewind is supported so we fail early
On 2014/11/12 18:00:12, scroggo wrote:
> decoders*
> 
> (I know this duplicating an old typo, but as long as we're moving the comment,
> please remove the error.)

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:113: #if !defined(SK_BUILD_FOR_MAC) &&
!defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) &&
!defined(SK_BUILD_FOR_NACL)
On 2014/11/12 18:00:12, scroggo wrote:
> line too long

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:114: movie = SkGIFMovieCreate(stream);
On 2014/11/12 18:00:12, scroggo wrote:
> The old code does a rewind if movie is null.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_ktx.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_ktx.cpp:248: return SkImageDecoder::kKTX_Format;
On 2014/11/12 18:00:12, scroggo wrote:
> indent 4 spaces

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_ktx.cpp:254: if (SkDetectFormatKTXImageDecoder(stream)
== SkImageDecoder::kKTX_Format) {
On 2014/11/12 18:00:12, scroggo wrote:
> Why not call SkKTXFile::is_ktx?

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_ktx.cpp:255: return SkNEW(SkKTXImageDecoder);
On 2014/11/12 18:00:12, scroggo wrote:
> indent 4 spaces.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_ktx.cpp:325: return (SkImageEncoder::kKTX_Type == t) ?
SkNEW(SkKTXImageEncoder) : NULL;
On 2014/11/12 18:00:12, scroggo wrote:
> 4 spaces.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_libbmp.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libbmp.cpp:134: !memcmp(buffer, kBmpMagic,
sizeof(kBmpMagic))) {
On 2014/11/12 18:00:12, scroggo wrote:
> I don't think it's in the style guide, but this is harder to read since the
> second line of the if statement lines up with the code inside the block.
> 
> One thing that would help is to put the brace on the next line. (Or you could
> indent this line 8 spaces instead of 4.)

Right..

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_libico.cpp (left):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libico.cpp:28: 
On 2014/11/12 18:00:12, scroggo wrote:
> Why the delete?

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libjpeg.cpp:1374: SkImageDecoder::Format
SkDetectFormatJPEGImageDecoder(SkStreamRewindable* stream) {
On 2014/11/12 18:00:12, scroggo wrote:
> nit: The change is easier to follow without moving the function

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_libpng.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libpng.cpp:76: 
On 2014/11/12 18:00:13, scroggo wrote:
> Why the extra line?

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libpng.cpp:1102: !png_sig_cmp((png_bytep) buf,
(png_size_t)0, PNG_BYTES_TO_CHECK)) {
On 2014/11/12 18:00:13, scroggo wrote:
> nit: move brace to next line.
> 
> (Again, easier to follow if you hadn't moved the functions.)

Done. (Reverted the reordering.)

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_libpng.h (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_libpng.h:1: 
On 2014/11/12 18:00:13, scroggo wrote:
> nit: no need for this empty line.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_pkm.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_pkm.cpp:108: if (etc1_pkm_is_valid(buf)) {
On 2014/11/12 18:00:13, scroggo wrote:
> nit: why not &&?
Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_wbmp.h (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_wbmp.h:1: 
On 2014/11/12 18:00:13, scroggo wrote:
> No need for this blank line.

Done.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
File src/images/SkImageEncoder_Factory.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
src/images/SkImageEncoder_Factory.cpp:26: #if defined(SK_BUILD_FOR_WIN)
On 2014/11/12 18:00:13, scroggo wrote:
> On 2014/10/20 14:03:39, Kimmo Kinnunen wrote:
> > I can double-check the encoder order and fix potential compile problems on
> > windows, mac and iOS if the approach is acceptable..
> 
> FYI, we have compile bots for windows and mac that will run as part of the
> commit queue (or you can manually run them from the reitveld issue). I don't
see
> one for iOS though.

I know, but I don't have access to run the bots manually from reitveld..

> I think the approach is good. I sort of hate that we need the same ifdef
> conditions in two places, but I can't think of a better way.

Yeah. The condition list is not that complex, though, and probably doesn't
change that often. Doing preprocessor tricks like specifying the order as a
for-each macro would be possible but probably harder to maintain with very
little benefit.

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
File src/images/SkImageEncoder_argb.h (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
src/images/SkImageEncoder_argb.h:1: 
On 2014/11/12 18:00:13, scroggo wrote:
> no need for this line.

Done.

[scroggo, 2014-11-18 16:52:23]

Regarding the larger issue of, is this the right direction:

I like this change because it removes the requirement of including
"SkForceLinking.h". SkForceLinking gets around an annoying behavior of the
linker to strip out the decoders, which it thinks are not used. (If we can find
a better way to *not* strip out the decoders, potentially while still stripping
out things that are actually unused, I think we should do that instead.)

One nice thing about the current system is that we can add another decoder via
the registration function. Except thanks to the linker, you also have to update
SkForceLinking, and any program that uses the decoders needs to use
SkForceLinking as well.

But let's get to the heart of the issue. What is your goal? I think your end
game is to run Skia's testing tools on Chromium. I think this is a great idea.
But I don't see how this CL accomplishes that. I've looked at your document at
https://docs.google.com/document/d/1pUbdJ6OSNK--y2rSjPRkUMYCCx7jg6CjcLB_AjZyv...
, but that doesn't mention anything about image decoding, or explain how this
helps Skia get there.

The description of this issue mentions that it leaves open a way for clients
like Chromium to add encoders/decoders. What is that way? Why not use the
existing way?

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
File src/images/SkImageDecoder_Factory.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageDec...
src/images/SkImageDecoder_Factory.cpp:74: FactoryCreateFunc createFunc =
gDecoderFactoryFuncs[i].createFunc;
On 2014/11/18 08:29:44, Kimmo Kinnunen wrote:
> To me, having two three-line functions per decoder is satisfyingly simple
> approach. I don't feel that strongly about it, so if you want, I can add some
> more lines in form of class declarations.

The issue isn't so much having two three-line functions per decoder so much as
having
two (hopefully!) identical three-line functions per decoder. But you do raise
good points about my suggestion.

> I changed below call to createFunc with following:
> if (detectFormatFunc(stream) != kUnknown) {
>   codec = createFunc(stream)
> }
> 
> This way one can remove the if statements from the create functions. Would
this
> suffice?

It looks like you actually now say:

format = detectFormatFunc(stream);
if (format != kUnknown) {
    codec = createFunc(format);
}

It seems odd that createFunc has a parameter that is only used in an assert in
debug mode.

> - Introduce problems with mutable state: threading issues, global
initialization
> issues, writable data (the issues we try to solve with this patch in the first
> place)

I'm looking back at your CL description, and it's not clear these are things
you're
trying to solve with this patch. What are the issues, and how are they solved?

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
File src/images/SkImageEncoder_Factory.cpp (right):

https://chromiumcodereview.appspot.com/670453002/diff/1/src/images/SkImageEnc...
src/images/SkImageEncoder_Factory.cpp:26: #if defined(SK_BUILD_FOR_WIN)
On 2014/11/18 08:29:45, Kimmo Kinnunen wrote:
> On 2014/11/12 18:00:13, scroggo wrote:
> > On 2014/10/20 14:03:39, Kimmo Kinnunen wrote:
> > > I can double-check the encoder order and fix potential compile problems on
> > > windows, mac and iOS if the approach is acceptable..
> > 
> > FYI, we have compile bots for windows and mac that will run as part of the
> > commit queue (or you can manually run them from the reitveld issue). I don't
> see
> > one for iOS though.
> 
> I know, but I don't have access to run the bots manually from reitveld..

That's annoying. Sorry, I didn't realize you couldn't do that...
> 
> > I think the approach is good. I sort of hate that we need the same ifdef
> > conditions in two places, but I can't think of a better way.
> 
> Yeah. The condition list is not that complex, though, and probably doesn't
> change that often. Doing preprocessor tricks like specifying the order as a
> for-each macro would be possible but probably harder to maintain with very
> little benefit.

Actually, I can think of a better way. Have a macro for each decoder, and then
determine whether that macro is true based on the OS:

#ifdef SK_BUILD_FOR_WIN
#define SK_BUILD_IMAGE_DECODER_WIC
#endif

(Then we could even define SK_BUILD_IMAGE_DECODER_WIC in gyp files based on the
operating system.)

[Kimmo Kinnunen, 2014-11-19 08:55:39]

On 2014/11/18 16:52:23, scroggo wrote:
> Regarding the larger issue of, is this the right direction:
> 
> I like this change because it removes the requirement of including
> "SkForceLinking.h". SkForceLinking gets around an annoying behavior of the
> linker to strip out the decoders, which it thinks are not used. (If we can
find
> a better way to *not* strip out the decoders, potentially while still
stripping
> out things that are actually unused, I think we should do that instead.)

Well, I guess it is such a good system that all these undefined behaviors wrt.
global constructor ordering and linker behavior is worth it.

> One nice thing about the current system is that we can add another decoder via
> the registration function. Except thanks to the linker, you also have to
update
> SkForceLinking, and any program that uses the decoders needs to use
> SkForceLinking as well.

Right, "except". Well, all I can say is that if the process of adding a decoder
is the thing you want to optimize, then global constructors might just be the
solution to go for.


> But let's get to the heart of the issue. What is your goal? I think your end
> game is to run Skia's testing tools on Chromium. I think this is a great idea.
> But I don't see how this CL accomplishes that.

The point is that from any point of view this change is a progression that fixes
real bugs. I beg the change to be looked from this perspective, and not even
hanging to any other dislike to bigger picture goals. This is how the cl
description is written.

Note: the only pov this change is not a progression is the perceived convenience
of adding a decoder. And I write "perceived", because I do not think at the
moment the benefit of current system is fulfilled. By posting this patch up for
consideration, I posit it is false thinking that paying for the current
__FORCE_LINKINGs and false #NOTEs in .gyp files to get SK_REGISTER_DECODER is
really more convenient. I think I have to admit the contrary now.

So I have spent quite lot of my time for fighting this change uphill. This is a
third-party developer trying to fix unit tests (of all things!), fighting over a
petty change over 4 weeks. I think I get the message that you do not want this
here, no need to stall this based on cl description nits and whatnot.

Let's just drop the end goal.

> I've looked at your document at
>
https://docs.google.com/document/d/1pUbdJ6OSNK--y2rSjPRkUMYCCx7jg6CjcLB_AjZyv...
> , but that doesn't mention anything about image decoding, or explain how this
> helps Skia get there.

The doc talks nothing but that, I believe. I've also explained it to you
multiple times over the numerous comments about the same subject. Same
executable can not have multiple implementations of SkImageDecoder. The way the
image decoders are hacked together, the implementation spreads all over the
place. This means that in order to compile SkImageDecoder_wic.cpp, for example,
you compile parts of SkImageDecoder.

Surely this goal can be achieved in multiple ways, including creating new
defines that cut SkImageDecoders magically, and then compiling the file multiple
times. I chose this approach, because this actually makes the code simpler (in
my opinion, that is) and fixes bugs. 

> The description of this issue mentions that it leaves open a way for clients
> like Chromium to add encoders/decoders. What is that way? Why not use the
> existing way?

Because using the existing way to implement client-extendable image decoders is
a mirage, just like the convenience of adding a decoder.

Now you have system where:
- Library has finite, unextensible set of possible image decoder types
(SkImageDecoder::Type)
--> Biggest use case (that I know, of course) for extensible image decoder list
is to implement image decoder for a _new_ type. If library would know about it,
then it would probably have an implementation of it

- Client can not know which image decoders the library has compiled in
--> If you use the extension system to implement already known type, then how do
you know the system does not have it?

- Client can not specify the order of image decoders
--> It's just a random order. If you masquerade your new decoder as an existing
type with an existing decoder, it's really not known which one will be selected.

If I had to guess, the API was not designed for client-extendable image
decoders. Having that possibility is a leak from the chase of the convenience of
the library implementing the decoders.

[scroggo, 2016-02-12 19:02:11]

On 2014/11/19 08:55:39, Kimmo Kinnunen wrote:
> On 2014/11/18 16:52:23, scroggo wrote:
> > Regarding the larger issue of, is this the right direction:
> > 
> > I like this change because it removes the requirement of including
> > "SkForceLinking.h". SkForceLinking gets around an annoying behavior of the
> > linker to strip out the decoders, which it thinks are not used. (If we can
> find
> > a better way to *not* strip out the decoders, potentially while still
> stripping
> > out things that are actually unused, I think we should do that instead.)
> 
> Well, I guess it is such a good system that all these undefined behaviors wrt.
> global constructor ordering and linker behavior is worth it.
> 
> > One nice thing about the current system is that we can add another decoder
via
> > the registration function. Except thanks to the linker, you also have to
> update
> > SkForceLinking, and any program that uses the decoders needs to use
> > SkForceLinking as well.
> 
> Right, "except". Well, all I can say is that if the process of adding a
decoder
> is the thing you want to optimize, then global constructors might just be the
> solution to go for.
> 
> 
> > But let's get to the heart of the issue. What is your goal? I think your end
> > game is to run Skia's testing tools on Chromium. I think this is a great
idea.
> > But I don't see how this CL accomplishes that.
> 
> The point is that from any point of view this change is a progression that
fixes
> real bugs. I beg the change to be looked from this perspective, and not even
> hanging to any other dislike to bigger picture goals. This is how the cl
> description is written.
> 
> Note: the only pov this change is not a progression is the perceived
convenience
> of adding a decoder. And I write "perceived", because I do not think at the
> moment the benefit of current system is fulfilled. By posting this patch up
for
> consideration, I posit it is false thinking that paying for the current
> __FORCE_LINKINGs and false #NOTEs in .gyp files to get SK_REGISTER_DECODER is
> really more convenient. I think I have to admit the contrary now.
> 
> So I have spent quite lot of my time for fighting this change uphill. This is
a
> third-party developer trying to fix unit tests (of all things!), fighting over
a
> petty change over 4 weeks. I think I get the message that you do not want this
> here, no need to stall this based on cl description nits and whatnot.
> 
> Let's just drop the end goal.
> 
> > I've looked at your document at
> >
>
https://docs.google.com/document/d/1pUbdJ6OSNK--y2rSjPRkUMYCCx7jg6CjcLB_AjZyv...
> > , but that doesn't mention anything about image decoding, or explain how
this
> > helps Skia get there.
> 
> The doc talks nothing but that, I believe. I've also explained it to you
> multiple times over the numerous comments about the same subject. Same
> executable can not have multiple implementations of SkImageDecoder. The way
the
> image decoders are hacked together, the implementation spreads all over the
> place. This means that in order to compile SkImageDecoder_wic.cpp, for
example,
> you compile parts of SkImageDecoder.
> 
> Surely this goal can be achieved in multiple ways, including creating new
> defines that cut SkImageDecoders magically, and then compiling the file
multiple
> times. I chose this approach, because this actually makes the code simpler (in
> my opinion, that is) and fixes bugs. 
> 
> > The description of this issue mentions that it leaves open a way for clients
> > like Chromium to add encoders/decoders. What is that way? Why not use the
> > existing way?
> 
> Because using the existing way to implement client-extendable image decoders
is
> a mirage, just like the convenience of adding a decoder.
> 
> Now you have system where:
> - Library has finite, unextensible set of possible image decoder types
> (SkImageDecoder::Type)
> --> Biggest use case (that I know, of course) for extensible image decoder
list
> is to implement image decoder for a _new_ type. If library would know about
it,
> then it would probably have an implementation of it
> 
> - Client can not know which image decoders the library has compiled in
> --> If you use the extension system to implement already known type, then how
do
> you know the system does not have it?
> 
> - Client can not specify the order of image decoders
> --> It's just a random order. If you masquerade your new decoder as an
existing
> type with an existing decoder, it's really not known which one will be
selected.
> 
> If I had to guess, the API was not designed for client-extendable image
> decoders. Having that possibility is a leak from the chase of the convenience
of
> the library implementing the decoders.

Phasing out SkImageDecoder