Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1459473007:
Updates the gyp file. (Closed)
DescriptionUpdates the gyp file.
Prototyped a dng codec.
BUG=skia:
Patch Set 1 #
Total comments: 32
Patch Set 2 : Updates the Raw Codec prototype. #
Total comments: 10
Messages
Total messages: 12 (2 generated)
msarett@google.com changed reviewers: + msarett@google.com
I didn't look too deeply yet, but added a few comments. You can respond to comments inline and then use "Publish and Mail Comments" to publish your drafts. https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp#newcode30 gyp/codec.gyp:30: '-fexceptions', Can you add a comment stating what this flag does and why we need it? https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp~ File gyp/codec.gyp~ (left): https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp~#oldcode1 gyp/codec.gyp~:1: # Copyright 2015 Google Inc. Can we delete all of the ~ files? I'm guessing they're just emacs backup files? https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode13 gyp/dng_sdk.gyp:13: 'cflags': ['-Wsign-compare'], I'm fine with this if it allows you to share a gyp file with elsewhere. But I think Skia already turns on this flag? https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode51 gyp/dng_sdk.gyp:51: '../third_party/externals/dng_sdk/source/RawEnvironment.h', I'm guessing there is a change to the DEPS file that goes along with this? https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode224 gyp/dng_sdk.gyp:224: ['OS != "linux"', { Does this work? I think skia defines "skia_os", but not OS? https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); What is your question about the swizzler here? It looks like you have everything set up properly. The swizzler can convert to the SkColorType specified in requestedInfo for you. However, there may be a performance benefit if the dng library can convert directly. The SkSwizzler::SrcConfig tells the swizzler the format of the src. It looks like above you are indicating that the source is RGBRGBRGB... The swizzler can convert that BGRA, RGBA, 565, depending on the request. Those are the color types that I think it makes sense to support.
https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp#newcode30 gyp/codec.gyp:30: '-fexceptions', On 2015/11/19 16:45:01, msarett wrote: > Can you add a comment stating what this flag does and why we need it? Yes, we need exceptions since the dng sdk uses them. I'll also add comments to the code. https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp~ File gyp/codec.gyp~ (left): https://codereview.chromium.org/1459473007/diff/1/gyp/codec.gyp~#oldcode1 gyp/codec.gyp~:1: # Copyright 2015 Google Inc. On 2015/11/19 16:45:01, msarett wrote: > Can we delete all of the ~ files? I'm guessing they're just emacs backup files? Yes I will do so. https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode13 gyp/dng_sdk.gyp:13: 'cflags': ['-Wsign-compare'], On 2015/11/19 16:45:01, msarett wrote: > I'm fine with this if it allows you to share a gyp file with elsewhere. > > But I think Skia already turns on this flag? Right, this is relic of our original gyp file. https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode51 gyp/dng_sdk.gyp:51: '../third_party/externals/dng_sdk/source/RawEnvironment.h', On 2015/11/19 16:45:01, msarett wrote: > I'm guessing there is a change to the DEPS file that goes along with this? Not yet, but that is the idea basically. https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode224 gyp/dng_sdk.gyp:224: ['OS != "linux"', { On 2015/11/19 16:45:01, msarett wrote: > Does this work? I think skia defines "skia_os", but not OS? Hmm, I don't know how it works. This is also from our original gyp file. I can build and execute on Linux, for other os I don't know. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); On 2015/11/19 16:45:01, msarett wrote: > What is your question about the swizzler here? It looks like you have > everything set up properly. > > The swizzler can convert to the SkColorType specified in requestedInfo for you. > However, there may be a performance benefit if the dng library can convert > directly. > > The SkSwizzler::SrcConfig tells the swizzler the format of the src. It looks > like above you are indicating that the source is RGBRGBRGB... The swizzler can > convert that BGRA, RGBA, 565, depending on the request. Those are the color > types that I think it makes sense to support. One of my problems here is that the code crashes using swizzler. Does my color setup on creation make sense? Can I just pass it nullptr?
scroggo@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1459473007/diff/1/gyp/dng_sdk.gyp#newcode224 gyp/dng_sdk.gyp:224: ['OS != "linux"', { On 2015/11/19 16:58:47, ebrauer wrote: > On 2015/11/19 16:45:01, msarett wrote: > > Does this work? I think skia defines "skia_os", but not OS? > > Hmm, I don't know how it works. This is also from our original gyp file. I can > build and execute on Linux, for other os I don't know. I think skia_os is the device, so if you're building for Android it is android, while OS might be your laptop/desktop. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:26: public: Skia uses four space indents (I probably should have pointed you at our style guide: https://skia.org/dev/contrib/style). https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:27: explicit DngStream(SkStream* stream) : data_(stream->getMemoryBase()), This is in the style guide, but this should look like: explicit DngStream(SkStream* stream) : fData(stream->getMemoryBase() , fDataSize(stream->getLength()) {} More importantly, this will not work for all streams. getMemoryBase is only implemented for a couple of subclasses, and getLength is not supported by Java streams (see b/8432093). We've removed all of our decoders' dependencies on it. If you actually *need* all of the data, you should copy it all from the stream if it's not already available (e.g. using SkCopyStreamToStorage in https://skia.googlesource.com/skia/+/master/src/core/SkStreamPriv.h). But that's dangerous, too. We have found that reading the entire stream can use too much memory (I thought we had an open bug for this, but I'm not sure where...). SkCodec is a replacement for an older image decoding class, and it was a deliberate choice in SkCodec to never rely on storing all the data if we can help it. It seems like this would be an even bigger concern with a RAW image, which is not compressed, IIUC. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:35: ThrowReadFile(); The rest of Skia does not use exceptions. Can we avoid them here? https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:38: std::memcpy(data, reinterpret_cast<const uint8*>(data_) + offset, count); Are you calling a particular version of memcpy? Everywhere else in Skia we just call "memcpy". https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:60: std::unique_ptr<dng_negative>* negative) { For historical reasons, Skia does not use unique_ptr directly. Our equivalent is SkAutoTDelete. Can you just return a dng_negative instead, though? I'm guessing if you return false, the client will not want the dng_negative. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); On 2015/11/19 16:58:47, ebrauer wrote: > On 2015/11/19 16:45:01, msarett wrote: > > What is your question about the swizzler here? It looks like you have > > everything set up properly. > > > > The swizzler can convert to the SkColorType specified in requestedInfo for > you. > > However, there may be a performance benefit if the dng library can convert > > directly. > > > > The SkSwizzler::SrcConfig tells the swizzler the format of the src. It looks > > like above you are indicating that the source is RGBRGBRGB... The swizzler > can > > convert that BGRA, RGBA, 565, depending on the request. Those are the color > > types that I think it makes sense to support. > > One of my problems here is that the code crashes using swizzler. Does my color > setup on creation make sense? Can I just pass it nullptr? Yes, you can pass nullptr for the SkPMColor* parameter. It is not needed for a source of kRGB. I'm guessing some other code is left out though. I see a commented out definition of src_row, but it appears to have no data in it? Where is the crash? https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h File src/codec/SkRawCodec.h (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h#newc... src/codec/SkRawCodec.h:43: //SkISize onGetScaledDimensions(float desiredScale) const override; This is only necessary if you support scales other than 1. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h#newc... src/codec/SkRawCodec.h:57: bool onDimensionsSupported(const SkISize&) override;*/ Again, only necessary if you support scales other than 1.
https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:26: public: On 2015/11/19 22:16:54, scroggo wrote: > Skia uses four space indents (I probably should have pointed you at our style > guide: https://skia.org/dev/contrib/style). Done. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:27: explicit DngStream(SkStream* stream) : data_(stream->getMemoryBase()), On 2015/11/19 22:16:54, scroggo wrote: > This is in the style guide, but this should look like: > > explicit DngStream(SkStream* stream) > : fData(stream->getMemoryBase() > , fDataSize(stream->getLength()) {} > > More importantly, this will not work for all streams. getMemoryBase is only > implemented for a couple of subclasses, and getLength is not supported by Java > streams (see b/8432093). We've removed all of our decoders' dependencies on it. > If you actually *need* all of the data, you should copy it all from the stream > if it's not already available (e.g. using SkCopyStreamToStorage in > https://skia.googlesource.com/skia/+/master/src/core/SkStreamPriv.h). But that's > dangerous, too. We have found that reading the entire stream can use too much > memory (I thought we had an open bug for this, but I'm not sure where...). > SkCodec is a replacement for an older image decoding class, and it was a > deliberate choice in SkCodec to never rely on storing all the data if we can > help it. It seems like this would be an even bigger concern with a RAW image, > which is not compressed, IIUC. Got it. I suppose the stream is read-forward only (like in Java)? Is it possible to have a buffered stream or do I need to buffer the data on the fly to enable us to read the data in random order? https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:35: ThrowReadFile(); On 2015/11/19 22:16:54, scroggo wrote: > The rest of Skia does not use exceptions. Can we avoid them here? The dng sdk uses them quite often. I don't see a good solution to this. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:38: std::memcpy(data, reinterpret_cast<const uint8*>(data_) + offset, count); On 2015/11/19 22:16:54, scroggo wrote: > Are you calling a particular version of memcpy? Everywhere else in Skia we just > call "memcpy". Done. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); On 2015/11/19 22:16:55, scroggo wrote: > On 2015/11/19 16:58:47, ebrauer wrote: > > On 2015/11/19 16:45:01, msarett wrote: > > > What is your question about the swizzler here? It looks like you have > > > everything set up properly. > > > > > > The swizzler can convert to the SkColorType specified in requestedInfo for > > you. > > > However, there may be a performance benefit if the dng library can convert > > > directly. > > > > > > The SkSwizzler::SrcConfig tells the swizzler the format of the src. It > looks > > > like above you are indicating that the source is RGBRGBRGB... The swizzler > > can > > > convert that BGRA, RGBA, 565, depending on the request. Those are the color > > > types that I think it makes sense to support. > > > > One of my problems here is that the code crashes using swizzler. Does my color > > setup on creation make sense? Can I just pass it nullptr? > > Yes, you can pass nullptr for the SkPMColor* parameter. It is not needed for a > source of kRGB. > > I'm guessing some other code is left out though. I see a commented out > definition of src_row, but it appears to have no data in it? > > Where is the crash? The crash happens in the swizzle call giving me a core dump. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h File src/codec/SkRawCodec.h (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h#newc... src/codec/SkRawCodec.h:43: //SkISize onGetScaledDimensions(float desiredScale) const override; On 2015/11/19 22:16:55, scroggo wrote: > This is only necessary if you support scales other than 1. Done. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.h#newc... src/codec/SkRawCodec.h:57: bool onDimensionsSupported(const SkISize&) override;*/ On 2015/11/19 22:16:55, scroggo wrote: > Again, only necessary if you support scales other than 1. Done.
https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); On 2015/11/20 11:43:44, ebrauer wrote: > On 2015/11/19 22:16:55, scroggo wrote: > > On 2015/11/19 16:58:47, ebrauer wrote: > > > On 2015/11/19 16:45:01, msarett wrote: > > > > What is your question about the swizzler here? It looks like you have > > > > everything set up properly. > > > > > > > > The swizzler can convert to the SkColorType specified in requestedInfo for > > > you. > > > > However, there may be a performance benefit if the dng library can convert > > > > directly. > > > > > > > > The SkSwizzler::SrcConfig tells the swizzler the format of the src. It > > looks > > > > like above you are indicating that the source is RGBRGBRGB... The > swizzler > > > can > > > > convert that BGRA, RGBA, 565, depending on the request. Those are the > color > > > > types that I think it makes sense to support. > > > > > > One of my problems here is that the code crashes using swizzler. Does my > color > > > setup on creation make sense? Can I just pass it nullptr? > > > > Yes, you can pass nullptr for the SkPMColor* parameter. It is not needed for a > > source of kRGB. > > > > I'm guessing some other code is left out though. I see a commented out > > definition of src_row, but it appears to have no data in it? > > > > Where is the crash? > > The crash happens in the swizzle call giving me a core dump. Again my fault. I must not use swizzler when the object was not created.
https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:27: explicit DngStream(SkStream* stream) : data_(stream->getMemoryBase()), On 2015/11/20 11:43:44, ebrauer wrote: > On 2015/11/19 22:16:54, scroggo wrote: > > This is in the style guide, but this should look like: > > > > explicit DngStream(SkStream* stream) > > : fData(stream->getMemoryBase() > > , fDataSize(stream->getLength()) {} > > > > More importantly, this will not work for all streams. getMemoryBase is only > > implemented for a couple of subclasses, and getLength is not supported by Java > > streams (see b/8432093). We've removed all of our decoders' dependencies on > it. > > If you actually *need* all of the data, you should copy it all from the stream > > if it's not already available (e.g. using SkCopyStreamToStorage in > > https://skia.googlesource.com/skia/+/master/src/core/SkStreamPriv.h). But > that's > > dangerous, too. We have found that reading the entire stream can use too much > > memory (I thought we had an open bug for this, but I'm not sure where...). > > SkCodec is a replacement for an older image decoding class, and it was a > > deliberate choice in SkCodec to never rely on storing all the data if we can > > help it. It seems like this would be an even bigger concern with a RAW image, > > which is not compressed, IIUC. > > Got it. I suppose the stream is read-forward only (like in Java)? Is it possible > to have a buffered stream or do I need to buffer the data on the fly to enable > us to read the data in random order? I'm not sure what you mean by "is it possible to have a buffered stream". From the Java API, there is a BufferedStream, but it does not give you random access (just reset, which rewinds to a mark, but only if less than the buffer has been read). We *could* supply you a buffered stream, but the other classes do not generally need it, so I would recommend you do your own buffering. https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:185: //swizzler->swizzle(dst_row, &src_row[0]); On 2015/11/20 12:57:57, ebrauer wrote: > On 2015/11/20 11:43:44, ebrauer wrote: > > On 2015/11/19 22:16:55, scroggo wrote: > > > On 2015/11/19 16:58:47, ebrauer wrote: > > > > On 2015/11/19 16:45:01, msarett wrote: > > > > > What is your question about the swizzler here? It looks like you have > > > > > everything set up properly. > > > > > > > > > > The swizzler can convert to the SkColorType specified in requestedInfo > for > > > > you. > > > > > However, there may be a performance benefit if the dng library can > convert > > > > > directly. > > > > > > > > > > The SkSwizzler::SrcConfig tells the swizzler the format of the src. It > > > looks > > > > > like above you are indicating that the source is RGBRGBRGB... The > > swizzler > > > > can > > > > > convert that BGRA, RGBA, 565, depending on the request. Those are the > > color > > > > > types that I think it makes sense to support. > > > > > > > > One of my problems here is that the code crashes using swizzler. Does my > > color > > > > setup on creation make sense? Can I just pass it nullptr? > > > > > > Yes, you can pass nullptr for the SkPMColor* parameter. It is not needed for > a > > > source of kRGB. > > > > > > I'm guessing some other code is left out though. I see a commented out > > > definition of src_row, but it appears to have no data in it? > > > > > > Where is the crash? > > > > The crash happens in the swizzle call giving me a core dump. > > Again my fault. I must not use swizzler when the object was not created. The swizzler was not created? That may mean we need to add support for some kind of conversion. I can help if you let me know why the swizzler creation failed.
https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:47: bool IsDng(SkStream* data) { Is it possible to do less work here? It appears you're doing a lot of work here to determine whether it is a valid DNG (I'm not familiar with DNG though; how much of the image does Parse parse?), but the point of this method is basically to know that this is *not* some other type of file. For example, in PNG, we just check the first few bytes for a magic signature. If those bytes do not match, we return and check the other formats. If they do match, we assume that it is a PNG file. If it turns out the image is corrupt in some way, we will fail later on, but won't need to check if the stream matches another format. Does DNG have a magic signature? https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:121: return IsDng(stream); Why didn't you just put the code for IsDng here? It looks like it is not used elsewhere?
Adding a few thoughts. https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:135: negative->DefaultCropSizeV().As_real64(), kRGBA_8888_SkColorType, We would normally recommend kN32_SkColorType here (which is equivalent to kBGRA or kRGBA depending on the platform). Is there a reason to always recommend RGBA? https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:170: std::unique_ptr<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler( Is the swizzler now working as you expect? https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.h File src/codec/SkRawCodec.h (right): https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.h#... src/codec/SkRawCodec.h:19: * This class implements the decoding for jpeg images Update this comment to say raw?
https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:27: explicit DngStream(SkStream* stream) : data_(stream->getMemoryBase()), On 2015/11/20 11:43:44, ebrauer wrote: > On 2015/11/19 22:16:54, scroggo wrote: > > This is in the style guide, but this should look like: > > > > explicit DngStream(SkStream* stream) > > : fData(stream->getMemoryBase() > > , fDataSize(stream->getLength()) {} > > > > More importantly, this will not work for all streams. getMemoryBase is only > > implemented for a couple of subclasses, and getLength is not supported by Java > > streams (see b/8432093). We've removed all of our decoders' dependencies on > it. > > If you actually *need* all of the data, you should copy it all from the stream > > if it's not already available (e.g. using SkCopyStreamToStorage in > > https://skia.googlesource.com/skia/+/master/src/core/SkStreamPriv.h). But > that's > > dangerous, too. We have found that reading the entire stream can use too much > > memory (I thought we had an open bug for this, but I'm not sure where...). > > SkCodec is a replacement for an older image decoding class, and it was a > > deliberate choice in SkCodec to never rely on storing all the data if we can > > help it. It seems like this would be an even bigger concern with a RAW image, > > which is not compressed, IIUC. > > Got it. I suppose the stream is read-forward only (like in Java)? More to the point - the stream may actually *be* a Java stream, which yes, is read-forward only.
This cl is deprecated. We continue to work on https://codereview.chromium.org/1520403003/ https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:47: bool IsDng(SkStream* data) { On 2015/11/24 14:46:05, scroggo wrote: > Is it possible to do less work here? It appears you're doing a lot of work here > to determine whether it is a valid DNG (I'm not familiar with DNG though; how > much of the image does Parse parse?), but the point of this method is basically > to know that this is *not* some other type of file. > > For example, in PNG, we just check the first few bytes for a magic signature. If > those bytes do not match, we return and check the other formats. If they do > match, we assume that it is a PNG file. If it turns out the image is corrupt in > some way, we will fail later on, but won't need to check if the stream matches > another format. > > Does DNG have a magic signature? To identify a valid DNG we would require this code, but we have a better solution in https://codereview.chromium.org/1520403003/ https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:121: return IsDng(stream); On 2015/11/24 14:46:05, scroggo wrote: > Why didn't you just put the code for IsDng here? It looks like it is not used > elsewhere? We are going to rework this as part of https://codereview.chromium.org/1520403003/ https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:135: negative->DefaultCropSizeV().As_real64(), kRGBA_8888_SkColorType, On 2015/11/25 15:15:23, msarett wrote: > We would normally recommend kN32_SkColorType here (which is equivalent to kBGRA > or kRGBA depending on the platform). Is there a reason to always recommend > RGBA? We changed it in https://codereview.chromium.org/1520403003/ https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:170: std::unique_ptr<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler( On 2015/11/25 15:15:23, msarett wrote: > Is the swizzler now working as you expect? It does what I expect, thanks. https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.h File src/codec/SkRawCodec.h (right): https://codereview.chromium.org/1459473007/diff/20001/src/codec/SkRawCodec.h#... src/codec/SkRawCodec.h:19: * This class implements the decoding for jpeg images On 2015/11/25 15:15:23, msarett wrote: > Update this comment to say raw? Done in https://codereview.chromium.org/1520403003/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
