Differentiate between sRGBGamma and 2Dot2Gamma

src/codec/SkPngCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmap.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
-#include "SkColorSpace.h"
+#include "SkColorSpace_Base.h"
 #include "SkColorTable.h"
 #include "SkMath.h"
 #include "SkOpts.h"
 #include "SkPngCodec.h"
 #include "SkSize.h"
 #include "SkStream.h"
 #include "SkSwizzler.h"
 #include "SkTemplates.h"
 #include "SkUtils.h"
 
@@ skipping 194 matching lines @@
 
         // FIXME (msarett): Here we are treating XYZ values as D50 even though the color
         //                  temperature is unspecified.  I suspect that this assumption
         //                  is most often ok, but we could also calculate the color
         //                  temperature (D value) and then convert the XYZ to D50.  Maybe
         //                  we should add a new constructor to SkColorSpace that accepts
         //                  XYZ with D-Unkown?
         for (int i = 0; i < 9; i++) {
             toXYZD50[i] = png_fixed_point_to_float(XYZ[i]);
         }
+        SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
+        mat.set3x3ColMajorf(toXYZD50);
 
         if (PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma)) {
             float value = png_inverted_fixed_point_to_float(gamma);
             gammas[0] = value;
             gammas[1] = value;
             gammas[2] = value;
 
         } else {
-            // Default to sRGB (gamma = 2.2f) if the image has color space information,
+            // Default to sRGB gamma if the image has color space information,
             // but does not specify gamma.
-            gammas[0] = 2.2f;
-            gammas[1] = 2.2f;
-            gammas[2] = 2.2f;
+            return SkColorSpace::NewRGB(SkColorSpace::kSRGB_GammaNamed, mat);
         }
 
-        SkMatrix44 mat(SkMatrix44::kUninitialized_Constructor);
-        mat.set3x3ColMajorf(toXYZD50);
-        return SkColorSpace::NewRGB(gammas, mat);
+        return SkColorSpace_Base::NewRGB(gammas, mat);

[Brian Osman, 2016/06/15 13:26:59]

Nit: Now that we're returning sRGB in the else case above, I'd move this line
into the previous 'if' block, to make the control flow symmetric/clearer.

[msarett, 2016/06/15 14:33:48]

Yup that makes a lot more sense.  Done.

     }
 
     // Last, check for gamma.
     if (PNG_INFO_gAMA == png_get_gAMA_fixed(png_ptr, info_ptr, &gamma)) {
 
         // Guess a default value for cHRM?  Or should we just give up?

[Brian Osman, 2016/06/15 13:26:59]

As I read this code and think about it more, I get conflicted. My gut says that
we should be guessing sRGB gamut for many of these images (those that just have
gAMA chunk, and those with no information at all). Or perhaps we guess sRGB
gamut if it has gAMA, and it's nearly 2.2?

It seems like a balancing act between trying to be clever, and making the
behavior too much of a black box. Of course, if people are creating new content
and don't like the results - they ought to embed proper profile data in their
images. It's probably more helpful to handle the huge amount of content that
already exists and isn't being maintained by anyone - so I vote for guessing
sRGB whenever we're not sure.

[msarett, 2016/06/15 14:33:48]

Yeah I'd agree we should guess sRGB here.  Nobody ever seems to specify a gamma
of 1...

Done.

         // Here we use the identity matrix as a default.
 
         // Set the gammas.
         float value = png_inverted_fixed_point_to_float(gamma);
         gammas[0] = value;
         gammas[1] = value;
         gammas[2] = value;
 
-        return SkColorSpace::NewRGB(gammas, SkMatrix44::I());
+        return SkColorSpace_Base::NewRGB(gammas, SkMatrix44::I());
     }
 
 #endif // LIBPNG >= 1.6
 
     // Finally, what should we do if there is no color space information in the PNG?
     // The specification says that this indicates "gamma is unknown" and that the
     // "color is device dependent".  I'm assuming we can represent this with NULL.
     // But should we guess sRGB?  Most images are sRGB, even if they don't specify.
     return nullptr;
 }
@@ skipping 538 matching lines @@
     SkCodec* outCodec;
     if (read_header(stream, chunkReader, &outCodec, nullptr, nullptr)) {
         // Codec has taken ownership of the stream.
         SkASSERT(outCodec);
         streamDeleter.release();
         return outCodec;
     }
 
     return nullptr;
 }