Remove image decoder and encoder autoregistration

src/images/SkImageDecoder_Factory.cpp

+/*
+ * Copyright 2013 The Android Open Source Project

[scroggo, 2014/11/12 18:00:12]

2014 Google Inc

[Kimmo Kinnunen, 2014/11/18 08:29:44]

Done.

+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkErrorInternals.h"
+#include "SkImageDecoder.h"
+#include "SkStream.h"
+
+#include "SkImageDecoder_libjpeg.h"
+#include "SkImageDecoder_libwebp.h"
+#include "SkImageDecoder_libico.h"
+#include "SkImageDecoder_libbmp.h"
+#include "SkImageDecoder_wbmp.h"
+
+#if defined(SK_BUILD_FOR_WIN)
+#include "SkImageDecoder_WIC.h"
+#endif
+#if defined(SK_BUILD_FOR_MAC) || defined(SK_BUILD_FOR_IOS)
+#include "SkImageDecoder_CG.h"
+#endif
+#if !defined(SK_BUILD_FOR_MAC) && !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN)
+#include "SkImageDecoder_libpng.h"
+#endif
+#if !defined(SK_BUILD_FOR_MAC) && !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) && !defined(SK_BUILD_FOR_NACL)

[scroggo, 2014/11/12 18:00:12]

line too long.

[Kimmo Kinnunen, 2014/11/18 08:29:44]

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

+#include "SkImageDecoder_libgif.h"
+#include "SkMovie_gif.h"
+#endif
+#if !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK)
+#include "SkImageDecoder_astc.h"
+#include "SkImageDecoder_ktx.h"
+#include "SkImageDecoder_pkm.h"
+#endif
+
+// This file is used for registration of SkImageDecoders. It also holds a function
+// for checking all the the registered SkImageDecoders for one that matches an
+// input SkStreamRewindable.
+
+typedef SkImageDecoder* (*FactoryCreateFunc)(SkStreamRewindable*);
+typedef SkImageDecoder::Format (*FactoryDetectFormatFunc)(SkStreamRewindable*);
+static const struct {
+    FactoryCreateFunc createFunc;
+    FactoryDetectFormatFunc getFormatFunc;
+} gDecoderFactoryFuncs[] = {

[scroggo, 2014/11/12 18:00:12]

Could you add a comment here that the order is meaningful? (Like the one removed
from images.gyp?)

[Kimmo Kinnunen, 2014/11/18 08:29:44]

Done.

+#if defined(SK_BUILD_FOR_WIN)
+    { SkCreateImageDecoder_WIC, SkDetectFormatImageDecoder_WIC },
+#endif
+#if defined(SK_BUILD_FOR_MAC) || defined(SK_BUILD_FOR_IOS)
+    { SkCreateImageDecoder_CG, SkDetectFormatImageDecoder_CG },
+#endif
+#if !defined(SK_BUILD_FOR_MAC) && !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN)
+    { SkCreatePNGImageDecoder, SkDetectFormatPNGImageDecoder },
+#endif
+    { SkCreateJPEGImageDecoder, SkDetectFormatJPEGImageDecoder },
+    { SkCreateWEBPImageDecoder, SkDetectFormatWEBPImageDecoder },
+    { SkCreateICOImageDecoder, SkDetectFormatICOImageDecoder },
+#if !defined(SK_BUILD_FOR_MAC) && !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) && !defined(SK_BUILD_FOR_NACL)

[scroggo, 2014/11/12 18:00:12]

line too long

[Kimmo Kinnunen, 2014/11/18 08:29:44]

Done.

+    { SkCreateGIFImageDecoder, SkDetectFormatGIFImageDecoder },
+#endif
+    { SkCreateBMPImageDecoder, SkDetectFormatBMPImageDecoder },
+#if !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK)
+    { SkCreateASTCImageDecoder, SkDetectFormatASTCImageDecoder },
+    { SkCreateKTXImageDecoder, SkDetectFormatKTXImageDecoder },
+    { SkCreatePKMImageDecoder, SkDetectFormatPKMImageDecoder },
+#endif
+    { SkCreateWBMPImageDecoder, SkDetectFormatWBMPImageDecoder }
+};
+
+SkImageDecoder* SkImageDecoder::Factory(SkStreamRewindable* stream) {
+    SkImageDecoder* codec = NULL;
+    for (size_t i = 0; i < SK_ARRAY_COUNT(gDecoderFactoryFuncs); ++i) {
+        FactoryCreateFunc createFunc = gDecoderFactoryFuncs[i].createFunc;

[scroggo, 2014/11/12 18:00:12]

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?

[Kimmo Kinnunen, 2014/11/18 08:29:44]

...
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?

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

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.
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.
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?

+        codec = createFunc(stream);
+        // we rewind here, because we promise later when we call "decode", that
+        // the stream will be at its beginning.
+        bool rewindSuceeded = stream->rewind();
+
+        // our image decoder's require that rewind is supported so we fail early

[scroggo, 2014/11/12 18:00:12]

decoders*

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

[Kimmo Kinnunen, 2014/11/18 08:29:44]

Done.

+        // if we are given a stream that does not support rewinding.
+        if (!rewindSuceeded) {
+            SkDEBUGF(("Unable to rewind the image stream."));
+            SkDELETE(codec);
+            return NULL;
+        }
+
+        if (codec) {
+            return codec;
+        }
+    }
+    return NULL;
+}
+
+SkImageDecoder::Format SkImageDecoder::GetStreamFormat(SkStreamRewindable* stream) {
+    for (size_t i = 0; i < SK_ARRAY_COUNT(gDecoderFactoryFuncs); ++i) {
+        FactoryDetectFormatFunc detectFormatFunc = gDecoderFactoryFuncs[i].getFormatFunc;
+        Format format = detectFormatFunc(stream);
+        if (!stream->rewind()) {
+            SkErrorInternals::SetError(kInvalidOperation_SkError,
+                                       "Unable to rewind the image stream\n");
+            return kUnknown_Format;
+        }
+        if (format != kUnknown_Format) {
+            return format;
+        }
+    }
+    return kUnknown_Format;
+}
+
+SkMovie* SkMovie::DecodeStream(SkStreamRewindable* stream) {
+    SkMovie* movie = NULL;
+#if !defined(SK_BUILD_FOR_MAC) && !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_WIN) && !defined(SK_BUILD_FOR_NACL)

[scroggo, 2014/11/12 18:00:12]

line too long

[Kimmo Kinnunen, 2014/11/18 08:29:45]

Done.

+    movie = SkGIFMovieCreate(stream);

[scroggo, 2014/11/12 18:00:12]

The old code does a rewind if movie is null.

[Kimmo Kinnunen, 2014/11/18 08:29:45]

Done.

+#endif
+    return movie;
+}