Issue 1520403003: Prototype of RAW decoding in Skia.

yujieqin

Description was changed from ========== Prototype of RAW decoding in Skia. Note: * Need to ...

5 years ago (2015-12-14 20:05:10 UTC) #1

yujieqin

Description was changed from ========== Prototype of RAW decoding in Skia. Note: * Need to ...

5 years ago (2015-12-14 20:06:45 UTC) #2

djsollen

https://codereview.chromium.org/1520403003/diff/1/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1520403003/diff/1/dm/DM.cpp#newcode491 dm/DM.cpp:491: "JPG", "JPEG", "PNG", "WEBP", "ARW", "CR2", "DNG", "NEF", "NRW", ...

5 years ago (2015-12-16 20:35:34 UTC) #4

scroggo

I haven't looked over this whole CL in depth. It looks to be very similar ...

5 years ago (2015-12-18 15:56:27 UTC) #6

msarett

Haven't looked over SkRawCodec.cpp in depth yet. https://codereview.chromium.org/1520403003/diff/1/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/1/src/codec/SkCodec.cpp#newcode91 src/codec/SkCodec.cpp:91: // Try ...

5 years ago (2015-12-18 16:26:13 UTC) #8

scroggo

https://codereview.chromium.org/1520403003/diff/1/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/1/src/codec/SkCodec.cpp#newcode91 src/codec/SkCodec.cpp:91: // Try read more for RAW cases. On 2015/12/18 ...

5 years ago (2015-12-18 16:47:06 UTC) #9

yujieqin

We got a new patch set 2, which hopefully addressed all comments from current & ...

4 years, 11 months ago (2016-01-06 18:45:44 UTC) #10

yujieqin

We got a new patch set 2, which hopefully addressed all comments from current & ...

4 years, 11 months ago (2016-01-06 18:47:19 UTC) #11

scroggo

https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp#newcode518 dm/DM.cpp:518: "bmp", "gif", "jpg", "jpeg", "png", "webp", "ktx", "astc", "wbmp", ...

4 years, 11 months ago (2016-01-06 22:30:21 UTC) #12

msarett

https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp#newcode74 gyp/codec.gyp:74: 'libjpeg-turbo-selector.gyp:libjpeg-turbo-selector', We ought to start building libjpeg-turbo as a ...

4 years, 11 months ago (2016-01-06 22:50:25 UTC) #13

adaubert

https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp#newcode74 gyp/codec.gyp:74: 'libjpeg-turbo-selector.gyp:libjpeg-turbo-selector', On 2016/01/06 22:50:24, msarett wrote: > We ought ...

4 years, 11 months ago (2016-01-07 08:28:41 UTC) #14

ebrauer

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

4 years, 11 months ago (2016-01-07 09:05:16 UTC) #15

ebrauer

https://codereview.chromium.org/1520403003/diff/20001/gyp/piex.gyp File gyp/piex.gyp (right): https://codereview.chromium.org/1520403003/diff/20001/gyp/piex.gyp#newcode35 gyp/piex.gyp:35: '../third_party/externals/piex/src/tiff_parser.cc', On 2016/01/06 22:30:20, scroggo wrote: > This supports ...

4 years, 11 months ago (2016-01-07 09:05:17 UTC) #16

yujieqin

https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp#newcode518 dm/DM.cpp:518: "bmp", "gif", "jpg", "jpeg", "png", "webp", "ktx", "astc", "wbmp", ...

4 years, 11 months ago (2016-01-07 09:22:26 UTC) #17

yujieqin

https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1520403003/diff/20001/dm/DM.cpp#newcode518 dm/DM.cpp:518: "bmp", "gif", "jpg", "jpeg", "png", "webp", "ktx", "astc", "wbmp", ...

4 years, 11 months ago (2016-01-07 09:22:27 UTC) #18

scroggo

https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp#newcode177 src/codec/SkRawCodec.cpp:177: SkDngStream dngStream(fDngLength, fDngStorage); On 2016/01/07 at 09:22:26, yujieqin wrote: ...

4 years, 11 months ago (2016-01-07 13:53:20 UTC) #19

https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:177: SkDngStream dngStream(fDngLength, fDngStorage);
On 2016/01/07 at 09:22:26, yujieqin wrote:
> On 2016/01/06 22:30:20, scroggo wrote:
> > nit: this should be indented (although I think it would be better to factor
out
> > the exception try/catch block).
> 
> Could you explain a bit more here about:
> * indented

try {
    SkDngStream dngStream(fDngLength, fDngStorage); // indented four spaces from
"try"
    dng_host host; // just like every other line inside this try block

> * factor out the try/catch

I imagined a mini library that hid the try/catch from this code, and instead
returned a boolean (or a pointer to an object, with null on failure/exception).
If this (i.e. the raw_codec target) makes Derek happy, though, I'm happy, so
long as we include some build guards so a client can build without exceptions.
e.g.

#ifdef SK_CODEC_DECODES_RAW // only defined if SK_HAS_EXCEPTIONS is defined
#include "SkRawCodec.h"
#endif

...

#ifdef SK_CODEC_DECODES_RAW
codec.reset(SkRawCodec::NewFromStream(...)
#endif

etc.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:91: if (!codec) {
I think this is a TAB? We use spaces instead.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:179: SkDngStream dngStream(fDngStorage, fDngLength);
Oh, I see the problem - this is a tab, but it should be indented using spaces.

(I've recently switched to the new UI for the code review too, which does not
distinguish between tabs and spaces. Looking in the old UI, I can see that it is
marked with a red ">>" signifying a tab. So it probably looks correct in your
editor, but does not line up properly in the new UI.)

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:185: !PrepareStage3(&dngStream, &host, &info,
negative.get()))
I think this can fit on one line now? Then you can leave the brace on the same
line as well:

if (!negative || !PrepareStage3(...)) {

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:218: } catch (...) {
Again, this uses a TAB, but it should be spaces.

yujieqin

PTAL https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp#newcode177 src/codec/SkRawCodec.cpp:177: SkDngStream dngStream(fDngLength, fDngStorage); On 2016/01/07 13:53:20, scroggo wrote: ...

4 years, 11 months ago (2016-01-07 15:52:02 UTC) #20

PTAL

https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/20001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:177: SkDngStream dngStream(fDngLength, fDngStorage);
On 2016/01/07 13:53:20, scroggo wrote:
> On 2016/01/07 at 09:22:26, yujieqin wrote:
> > On 2016/01/06 22:30:20, scroggo wrote:
> > > nit: this should be indented (although I think it would be better to
factor
> out
> > > the exception try/catch block).
> > 
> > Could you explain a bit more here about:
> > * indented
> 
> try {
>     SkDngStream dngStream(fDngLength, fDngStorage); // indented four spaces
from
> "try"
>     dng_host host; // just like every other line inside this try block
> 
> > * factor out the try/catch
> 
> I imagined a mini library that hid the try/catch from this code, and instead
> returned a boolean (or a pointer to an object, with null on
failure/exception).
> If this (i.e. the raw_codec target) makes Derek happy, though, I'm happy, so
> long as we include some build guards so a client can build without exceptions.
> e.g.
> 
> #ifdef SK_CODEC_DECODES_RAW // only defined if SK_HAS_EXCEPTIONS is defined
> #include "SkRawCodec.h"
> #endif
> 
> ...
> 
> #ifdef SK_CODEC_DECODES_RAW
> codec.reset(SkRawCodec::NewFromStream(...)
> #endif
> 
> etc.

We added the SK_CODEC_DECODES_RAW in codec.gyp:raw_codec. If one removes the
dependency to codec.gyp:raw_codec in codec.gyp:skia_codec, everything builds
fine and RAW support is disabled.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:91: if (!codec) {
On 2016/01/07 13:53:20, scroggo wrote:
> I think this is a TAB? We use spaces instead.

Done.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:179: SkDngStream dngStream(fDngStorage, fDngLength);
On 2016/01/07 13:53:20, scroggo wrote:
> Oh, I see the problem - this is a tab, but it should be indented using spaces.
> 
> (I've recently switched to the new UI for the code review too, which does not
> distinguish between tabs and spaces. Looking in the old UI, I can see that it
is
> marked with a red ">>" signifying a tab. So it probably looks correct in your
> editor, but does not line up properly in the new UI.)

Got it and fixed.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:185: !PrepareStage3(&dngStream, &host, &info,
negative.get()))
On 2016/01/07 13:53:20, scroggo wrote:
> I think this can fit on one line now? Then you can leave the brace on the same
> line as well:
> 
> if (!negative || !PrepareStage3(...)) {

Done.

https://codereview.chromium.org/1520403003/diff/60001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:218: } catch (...) {
On 2016/01/07 13:53:20, scroggo wrote:
> Again, this uses a TAB, but it should be spaces.

Done.

msarett

https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/20001/gyp/codec.gyp#newcode74 gyp/codec.gyp:74: 'libjpeg-turbo-selector.gyp:libjpeg-turbo-selector', On 2016/01/07 08:28:41, adaubert wrote: > On 2016/01/06 ...

4 years, 11 months ago (2016-01-07 16:39:11 UTC) #21

scroggo

(Sorry, I thought I sent this earlier...) https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79 gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW', This ...

4 years, 11 months ago (2016-01-07 20:57:43 UTC) #22

(Sorry, I thought I sent this earlier...)

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW',
This is a step in the right direction. But it doesn't provide a way to avoid
this target altogether.

You'll need to create a condition (in gyp/common_conditions.gypi; it should be
named "skia_codec_decodes_raw" to match this one) that determines whether to
build the raw_codec_target. This will also determine whether
SK_CODEC_DECODES_RAW is defined. That way we can tell gyp whether to build with
it or not.

It may need to depend on some variable (that does not yet exist) that specifies
whether exceptions are allowed, but I'm fine to wait until we verify that we
actually still support platforms that do not allow exceptions.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: if (!codec && streamDeleter) {
I think this is redundant.

If codec is NULL, streamDeleter should not be NULL. Maybe this should
SkASSERT(streamDeleter) instead?

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:38: bool SafeAddToSizeT(T arg1, T arg2, size_t* result)
{
In Skia, we name static functions (or those in anonymous namespaces) as

safe_add_to_size_t

(I should have mentioned that for ReadDng etc)

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:51: dng_negative* ReadDng(dng_stream* stream, dng_host*
host, dng_info* info) {
This should be read_dng

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:85: bool PrepareStage3(dng_stream* stream, dng_host*
host, dng_info* info,
prepare_stage_3

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:110: class SkRawStream : public dng_stream, public
::piex::StreamInterface {
We usually avoid multiple inheritance in Skia (see
https://groups.google.com/a/google.com/d/msg/skia-dev/gyRP6pHa154/_1L6Hb2oHrYJ).
Unless this is okay because one (or even better, both?) of these is an
interface?

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:126: SkMemoryStream* CopyBuffer(size_t offset, size_t
size) {
style: (non-static) member functions are named like "copyBuffer". (I know some
of these are overrides, so you cannot change those.)

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:133: reinterpret_cast<const
uint8*>(fStreamBuffer.get()) + offset, size, true);
You can get rid of these casts by making fStreamBuffer an SkAutoTMalloc, from
SkTemplates.h

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:168: bool IncreaseStreamBufferSize(size_t size, bool
readToEnd = false) {
style: increaseStreamBufferSize

Also, I find this method confusing. It seems like what it does is read from the
internal stream into a buffer. Maybe a better name would be bufferMoreData?

The parameter name is also ambiguous. I think a better name would be newSize?

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:169: if (!readToEnd) {
nit: I find it weird to say:

if (!condition) {
  ...
} else {
  ...
}

rather than saying if(condition) and switching the blocks. Maybe that's just me
though.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:172: } else if (fWholeStreamRead) {  // request more
than available.
no need for "else" here. We would have returned if the first condition was true.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:176: if (fWholeStreamRead) {
It seems weird to me that this is different depending on readToEnd. Is it okay
to ignore size here?

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:183: readToEnd ? fReadBufferSize :
std::min(fReadBufferSize, sizeToRead);
nit: SkTMin

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:187: SkDynamicMemoryWStream tempStream;
I think this code is made more complex by the fact that you're trying to handle
two cases:

1. Increase to a specific size.

This case does not need the SkDynamicMemoryStream. Instead, you can reallocate
(since you know the final size) and read (once) into the new memory:

fStreamBuffer.realloc(newSize);
uint8_t* dst = fStreamBuffer.get() + fStreamBufferSize;
size_t bytesRead = fStream->read(dst, sizeToRead);
if (bytesRead < sizeToRead) {
  // I'm guessing we'll no longer need it, so we could go ahead and free
  // the buffer
  fStreamBuffer.free();
  return false;
}
fStreamBufferSize = newSize;
return true;

This may allocate more memory than necessary on failure, but I'm assuming we
should optimize for success, where this saves allocations and copying.

2. Read the rest of the stream:
Here you'll need to loop over the rest of the stream, assuming the length is not
known, like you do here (and it's a little simpler because you don't need to
worry about the other case). But the code is more or less copied from
SkCopyStreamToStorage. What do you think about refactoring that so you can share
code?

FWIW, I think it is fine to special case where an SkMemoryStream is being used.
It might be better to leave that as a TODO though, so that running our tests
(which use SkMemoryStreams) more closely matches the common case on Android.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:297: if (fRawStream) {
I think it's safe to delete fRawStream even if it's null?

yujieqin

* Add DNG scaling: now the DNG image could be rendered earlier when the onGetScaledDimensions() ...

4 years, 11 months ago (2016-01-08 14:00:12 UTC) #23

* Add DNG scaling: now the DNG image could be rendered earlier when the
onGetScaledDimensions() is called.
* Address comments.

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW',
So we can postpone the change you mentioned for now, and keep current solution?

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode95
gyp/codec.gyp:95: 'cflags':[
On 2016/01/07 16:39:11, msarett wrote:
> IIUC, cflags is not respected on all platforms (maybe Windows or something
> ignores it, I can't remember).
> 
> But if you write the following, I think it will be portable:
> 
> defines: [
>   'SK_CODEC_DECODES_RAW',
> ]

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: if (!codec && streamDeleter) {
msarett@, yes, we run into once this issue that other codec.NewFromStream()
already took the ownership but failed to create the codec. In this case, the
(codec == nullptr) and (streamDeleter == nullptr).

scroggo@, do you think the case mentioned above is actually an assert? Just
asking because I think the current skia does not treat this case as an
assertion.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:94: #ifdef SK_CODEC_DECODES_RAW
On 2016/01/07 16:39:11, msarett wrote:
> nit: Can we surround the entire if statement with the #ifdef?

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:38: bool SafeAddToSizeT(T arg1, T arg2, size_t* result)
{
On 2016/01/07 20:57:42, scroggo wrote:
> In Skia, we name static functions (or those in anonymous namespaces) as
> 
> safe_add_to_size_t
> 
> (I should have mentioned that for ReadDng etc)

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:51: dng_negative* ReadDng(dng_stream* stream, dng_host*
host, dng_info* info) {
On 2016/01/07 20:57:43, scroggo wrote:
> This should be read_dng

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:85: bool PrepareStage3(dng_stream* stream, dng_host*
host, dng_info* info,
On 2016/01/07 20:57:42, scroggo wrote:
> prepare_stage_3

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:110: class SkRawStream : public dng_stream, public
::piex::StreamInterface {
On 2016/01/07 20:57:42, scroggo wrote:
> We usually avoid multiple inheritance in Skia (see
>
https://groups.google.com/a/google.com/d/msg/skia-dev/gyRP6pHa154/_1L6Hb2oHrYJ).
> Unless this is okay because one (or even better, both?) of these is an
> interface?

Both dng_stream and ::piex::StreamInterface are indeed interfaces.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:126: SkMemoryStream* CopyBuffer(size_t offset, size_t
size) {
On 2016/01/07 20:57:42, scroggo wrote:
> style: (non-static) member functions are named like "copyBuffer". (I know some
> of these are overrides, so you cannot change those.)

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:133: reinterpret_cast<const
uint8*>(fStreamBuffer.get()) + offset, size, true);
On 2016/01/07 20:57:42, scroggo wrote:
> You can get rid of these casts by making fStreamBuffer an SkAutoTMalloc, from
> SkTemplates.h

Nice, thanks! :)

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:168: bool IncreaseStreamBufferSize(size_t size, bool
readToEnd = false) {
On 2016/01/07 20:57:42, scroggo wrote:
> style: increaseStreamBufferSize
> 
> Also, I find this method confusing. It seems like what it does is read from
the
> internal stream into a buffer. Maybe a better name would be bufferMoreData?
> 
> The parameter name is also ambiguous. I think a better name would be newSize?

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:169: if (!readToEnd) {
On 2016/01/07 20:57:43, scroggo wrote:
> nit: I find it weird to say:
> 
> if (!condition) {
>   ...
> } else {
>   ...
> }
> 
> rather than saying if(condition) and switching the blocks. Maybe that's just
me
> though.

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:172: } else if (fWholeStreamRead) {  // request more
than available.
On 2016/01/07 20:57:43, scroggo wrote:
> no need for "else" here. We would have returned if the first condition was
true.

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:176: if (fWholeStreamRead) {
On 2016/01/07 20:57:43, scroggo wrote:
> It seems weird to me that this is different depending on readToEnd. Is it okay
> to ignore size here?

Yes, if readToEnd is "true", the newSize will be basically ignore. I added some
comment.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:183: readToEnd ? fReadBufferSize :
std::min(fReadBufferSize, sizeToRead);
On 2016/01/07 20:57:42, scroggo wrote:
> nit: SkTMin

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:187: SkDynamicMemoryWStream tempStream;
On 2016/01/07 20:57:42, scroggo wrote:
> I think this code is made more complex by the fact that you're trying to
handle
> two cases:
> 
> 1. Increase to a specific size.
> 
> This case does not need the SkDynamicMemoryStream. Instead, you can reallocate
> (since you know the final size) and read (once) into the new memory:
> 
> fStreamBuffer.realloc(newSize);
> uint8_t* dst = fStreamBuffer.get() + fStreamBufferSize;
> size_t bytesRead = fStream->read(dst, sizeToRead);
> if (bytesRead < sizeToRead) {
>   // I'm guessing we'll no longer need it, so we could go ahead and free
>   // the buffer
>   fStreamBuffer.free();
>   return false;
> }
> fStreamBufferSize = newSize;
> return true;
> 
> This may allocate more memory than necessary on failure, but I'm assuming we
> should optimize for success, where this saves allocations and copying.
> 
> 2. Read the rest of the stream:
> Here you'll need to loop over the rest of the stream, assuming the length is
not
> known, like you do here (and it's a little simpler because you don't need to
> worry about the other case). But the code is more or less copied from
> SkCopyStreamToStorage. What do you think about refactoring that so you can
share
> code?
> 
> FWIW, I think it is fine to special case where an SkMemoryStream is being
used.
> It might be better to leave that as a TODO though, so that running our tests
> (which use SkMemoryStreams) more closely matches the common case on Android.

Done.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:297: if (fRawStream) {
On 2016/01/07 20:57:42, scroggo wrote:
> I think it's safe to delete fRawStream even if it's null? 

Done.

msarett

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode232 src/codec/SkRawCodec.cpp:232: return new SkRawCodec(imageInfo, rawStream.detach(), host.release(), info.release(), negative.release()); nit: Line ...

4 years, 11 months ago (2016-01-08 14:38:34 UTC) #24

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:232: return new SkRawCodec(imageInfo,
rawStream.detach(), host.release(), info.release(), negative.release());
nit: Line is too long

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
I think here, you are assuming that getScaledDimensions() has already been
called, and then the client is calling getPixels() with the same size returned
by getScaledDimensions()?

In that case, I think this will work.

However, the client is not required to use the size returned by
getScaledDimensions().  I think this will break if the client calls
getScaledDimensions(), but then decides to call getPixels() on a different size.

I'm in favor of not repeating the render_dng_for_prefered_size() call, but to do
that, I think you'll need to make sure the current size of fDngImage matches the
request.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:290: const int preferedSize = SkTMax(dim.fWidth,
dim.fHeight);
Just for my curiosity, why does dng only need to know the max scaled dimension?

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:294: return this->getInfo().dimensions();
In general, if the exact requested scale is not supported, we try to suggest a
scale that is supported and is as close as we can get to the request.

When does "render_dng_for_prefered_size" fail?  Is this an error case?  Or can
we choose a closer match for the request?

Based on your implementation of onDimensionsSupported(), I think this is an
error case (so maybe the current implementation is fine?  or maybe we should
return (0, 0)?)

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:314: SkRawCodec::SkRawCodec(const SkImageInfo& srcInfo,
SkRawStream* stream, dng_host* host, dng_info* info,
nit: Line greater than 100 chars

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:316: : INHERITED(srcInfo, nullptr), fRawStream(stream),
fDngHost(host), fDngInfo(info), fDngNegative(negative) {}
nit: Line greater than 100 chars

scroggo

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79 gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW', On 2016/01/08 14:00:12, yujieqin wrote: > So we ...

4 years, 11 months ago (2016-01-08 17:12:26 UTC) #25

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW',
On 2016/01/08 14:00:12, yujieqin wrote:
> So we can postpone the change you mentioned for now, and keep current
solution?

You mean the change to factor out the exceptions? Yes, that is not needed. But
the change to add a condition is necessary.

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/60002/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:93: if (!codec && streamDeleter) {
On 2016/01/08 14:00:12, yujieqin wrote:
> msarett@, yes, we run into once this issue that other codec.NewFromStream()
> already took the ownership but failed to create the codec. In this case, the
> (codec == nullptr) and (streamDeleter == nullptr).
> 
> scroggo@, do you think the case mentioned above is actually an assert? Just
> asking because I think the current skia does not treat this case as an
> assertion.

My mistake. It is possible that the first few bytes told us it was a BMP (for
example), so we try to create a BMP codec. But the stream fails shortly
thereafter. BMP still deletes the stream (since we don't need to check if it's
something else), so both are NULL.

But I think you can make it the other way around:

if (streamDeleter) {
    SkASSERT(!codec);
    codec.reset(...);
}

If there is still a stream, to use, there should not be a codec already created.
If there was, we should already have a problem of use after free (streamDeleter
deletes it before codec keeps reading it).

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawAdapterC...
File src/codec/SkRawAdapterCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawAdapterC...
src/codec/SkRawAdapterCodec.cpp:16: SkISize
SkRawAdapterCodec::onGetSampledDimensions(int sampleSize) const {
This file is starting to look almost exactly like SkWebpAdapatorCodec.cpp.

Two differences:
- This one does not support subsets, but the code in that file will support
that: if a subset is requested, getSampledSubsetDimensions() will be called and
return (0,0), which result in a failure.
- This one does not check to make sure the dimensions are supported when there's
no subset either. I'm not sure whether it needs to or not.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:32: #include <vector>
Why do you need to include <vector>? I don't see any vectors?

Which makes me take another look at the rest of your includes. Do you need
cmath? We have a lot of our own functions in SkMath.h. Maybe you can use code
already supplied by Skia?

string.h is included by SkTypes. Do you need cstring here?

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:122: if (!safe_add_to_size_t(offset, size, &sum) ||
nit: I think these can fit on one line?

if (!safe_add_to_size_t || !bufferMoreData(sum)) {

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:133: if (!safe_add_to_size_t(offset, length, &sum) ||
one line?

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:134: !bufferMoreData(sum)) {
Sorry, something else I missed on previous reviews: in Skia, we use the this
pointer when calling member functions on the same object, e.g.

!this->bufferMoreData(sum)

here and elsewhere.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:162: bool bufferMoreData(size_t newSize, bool readToEnd
= false) {
Instead of having two mutually-exclusive parameters, why not make newSize == 0 a
sentinel that means readToEnd?

You could even define a constant:

// Sentinel to tell bufferMoreData to buffer the entire stream.
const size_t kReadToEnd = 0;

// If newSize is kReadToEnd, buffer the entire stream.
bool bufferMoreData(size_t newSize) {
  if (kReadToEnd == newSize) {
     ...
  }
  ...
}

// elsewhere:
this->bufferMoreData(kReadToEnd)

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:182: fStreamBufferSize = wholeStreamSize;
I think you can

  return true;

here. Then you do not need the else statement.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:232: return new SkRawCodec(imageInfo,
rawStream.detach(), host.release(), info.release(), negative.release());
On 2016/01/08 14:38:34, msarett wrote:
> nit: Line is too long

Also:

For historical reasons, SkAutoTDelete has two methods which do the same thing:
detach() and release().

(The history: SkAutoTDelete is older than unique_ptr, and we chose a different
name from the eventual unique_ptr. We added release() when we implemented it
with unique_ptr, and I think we'll eventually switch.)

I don't have a strong preference between the two (although maybe we should go
with release() for new code, to facilitate an eventual removal), but I think you
should be consistent on a single line of code.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:246: return kInvalidInput;
Arguably this could be kInvalidScale, although I don't know if you can tell the
difference.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:294: return this->getInfo().dimensions();
On 2016/01/08 14:38:34, msarett wrote:
> In general, if the exact requested scale is not supported, we try to suggest a
> scale that is supported and is as close as we can get to the request.
> 
> When does "render_dng_for_prefered_size" fail?  Is this an error case?  Or can
> we choose a closer match for the request?
> 
> Based on your implementation of onDimensionsSupported(), I think this is an
> error case (so maybe the current implementation is fine?  or maybe we should
> return (0, 0)?)

(On another note, this should be indented four spaces.)

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:316: : INHERITED(srcInfo, nullptr), fRawStream(stream),
fDngHost(host), fDngInfo(info), fDngNegative(negative) {}
FWIW, in Skia we often will put each member on its own line, e.g.

SkRawCodec::SkRawCodec(...)
    : INHERITED(srcInfo, nullptr)
    , fRawStream(stream)
    , ...

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.h#...
src/codec/SkRawCodec.h:59: * Called only by NewFromStream
nit: Takes ownership of all of its pointer parameters

scroggo

https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStream.cpp#newcode968 src/core/SkStream.cpp:968: // TODO: optimize for the special case when the ...

4 years, 11 months ago (2016-01-08 19:11:49 UTC) #26

yujieqin

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79 gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW', On 2016/01/08 17:12:25, scroggo wrote: > On 2016/01/08 ...

4 years, 11 months ago (2016-01-11 14:03:08 UTC) #27

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/60002/gyp/codec.gyp#newcode79
gyp/codec.gyp:79: '-DSK_CODEC_DECODES_RAW',
On 2016/01/08 17:12:25, scroggo wrote:
> On 2016/01/08 14:00:12, yujieqin wrote:
> > So we can postpone the change you mentioned for now, and keep current
> solution?
> 
> You mean the change to factor out the exceptions? Yes, that is not needed. But
> the change to add a condition is necessary.

We define the variable "skia_codec_decodes_raw" in common_variables.gypi. Then
add the condition for "SK_CODEC_DECODES_RAW" in common_conditions.gypi. Finally
includes common.gypi in codec.gyp. Is that correct?

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawAdapterC...
File src/codec/SkRawAdapterCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawAdapterC...
src/codec/SkRawAdapterCodec.cpp:16: SkISize
SkRawAdapterCodec::onGetSampledDimensions(int sampleSize) const {
On 2016/01/08 17:12:25, scroggo wrote:
> This file is starting to look almost exactly like SkWebpAdapatorCodec.cpp.
> 
> Two differences:
> - This one does not support subsets, but the code in that file will support
> that: if a subset is requested, getSampledSubsetDimensions() will be called
and
> return (0,0), which result in a failure.
> - This one does not check to make sure the dimensions are supported when
there's
> no subset either. I'm not sure whether it needs to or not.

These two do look similar to some extend, but, as you mentioned, there are still
differences regarding how to handle the subset. Right now, we think we can keep
it separated.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:32: #include <vector>
On 2016/01/08 17:12:25, scroggo wrote:
> Why do you need to include <vector>? I don't see any vectors?
> 
> Which makes me take another look at the rest of your includes. Do you need
> cmath? We have a lot of our own functions in SkMath.h. Maybe you can use code
> already supplied by Skia?
> 
> string.h is included by SkTypes. Do you need cstring here?

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:122: if (!safe_add_to_size_t(offset, size, &sum) ||
On 2016/01/08 17:12:25, scroggo wrote:
> nit: I think these can fit on one line?
> 
> if (!safe_add_to_size_t || !bufferMoreData(sum)) {

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:133: if (!safe_add_to_size_t(offset, length, &sum) ||
On 2016/01/08 17:12:25, scroggo wrote:
> one line?

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:134: !bufferMoreData(sum)) {
On 2016/01/08 17:12:25, scroggo wrote:
> Sorry, something else I missed on previous reviews: in Skia, we use the this
> pointer when calling member functions on the same object, e.g.
> 
> !this->bufferMoreData(sum)
> 
> here and elsewhere.

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:162: bool bufferMoreData(size_t newSize, bool readToEnd
= false) {
On 2016/01/08 17:12:26, scroggo wrote:
> Instead of having two mutually-exclusive parameters, why not make newSize == 0
a
> sentinel that means readToEnd?
> 
> You could even define a constant:
> 
> // Sentinel to tell bufferMoreData to buffer the entire stream.
> const size_t kReadToEnd = 0;
> 
> // If newSize is kReadToEnd, buffer the entire stream.
> bool bufferMoreData(size_t newSize) {
>   if (kReadToEnd == newSize) {
>      ...
>   }
>   ...
> }
> 
> // elsewhere:
> this->bufferMoreData(kReadToEnd)

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:182: fStreamBufferSize = wholeStreamSize;
On 2016/01/08 17:12:26, scroggo wrote:
> I think you can
> 
>   return true;
> 
> here. Then you do not need the else statement.

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:232: return new SkRawCodec(imageInfo,
rawStream.detach(), host.release(), info.release(), negative.release());
On 2016/01/08 17:12:25, scroggo wrote:
> On 2016/01/08 14:38:34, msarett wrote:
> > nit: Line is too long
> 
> Also:
> 
> For historical reasons, SkAutoTDelete has two methods which do the same thing:
> detach() and release().
> 
> (The history: SkAutoTDelete is older than unique_ptr, and we chose a different
> name from the eventual unique_ptr. We added release() when we implemented it
> with unique_ptr, and I think we'll eventually switch.)
> 
> I don't have a strong preference between the two (although maybe we should go
> with release() for new code, to facilitate an eventual removal), but I think
you
> should be consistent on a single line of code. 

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/08 14:38:34, msarett wrote:
> I think here, you are assuming that getScaledDimensions() has already been
> called, and then the client is calling getPixels() with the same size returned
> by getScaledDimensions()?
> 
> In that case, I think this will work.
> 
> However, the client is not required to use the size returned by
> getScaledDimensions().  I think this will break if the client calls
> getScaledDimensions(), but then decides to call getPixels() on a different
size.
> 
> I'm in favor of not repeating the render_dng_for_prefered_size() call, but to
do
> that, I think you'll need to make sure the current size of fDngImage matches
the
> request.

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:246: return kInvalidInput;
On 2016/01/08 17:12:26, scroggo wrote:
> Arguably this could be kInvalidScale, although I don't know if you can tell
the
> difference.

We think "kInvalidInput" should be correct for this case, because the ckeck for
the size is done below.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:290: const int preferedSize = SkTMax(dim.fWidth,
dim.fHeight);
On 2016/01/08 14:38:34, msarett wrote:
> Just for my curiosity, why does dng only need to know the max scaled
dimension?

The DNG SDK only need the long edge size, because it always preservers the
aspect ratio.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:294: return this->getInfo().dimensions();
On 2016/01/08 17:12:25, scroggo wrote:
> On 2016/01/08 14:38:34, msarett wrote:
> > In general, if the exact requested scale is not supported, we try to suggest
a
> > scale that is supported and is as close as we can get to the request.
> > 
> > When does "render_dng_for_prefered_size" fail?  Is this an error case?  Or
can
> > we choose a closer match for the request?
> > 
> > Based on your implementation of onDimensionsSupported(), I think this is an
> > error case (so maybe the current implementation is fine?  or maybe we should
> > return (0, 0)?)
> 
> (On another note, this should be indented four spaces.)

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:294: return this->getInfo().dimensions();
On 2016/01/08 14:38:34, msarett wrote:
> In general, if the exact requested scale is not supported, we try to suggest a
> scale that is supported and is as close as we can get to the request.
> 
> When does "render_dng_for_prefered_size" fail?  Is this an error case?  Or can
> we choose a closer match for the request?
> 
> Based on your implementation of onDimensionsSupported(), I think this is an
> error case (so maybe the current implementation is fine?  or maybe we should
> return (0, 0)?)

Yes, this is actually an error case.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:314: SkRawCodec::SkRawCodec(const SkImageInfo& srcInfo,
SkRawStream* stream, dng_host* host, dng_info* info,
On 2016/01/08 14:38:34, msarett wrote:
> nit: Line greater than 100 chars

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:316: : INHERITED(srcInfo, nullptr), fRawStream(stream),
fDngHost(host), fDngInfo(info), fDngNegative(negative) {}
On 2016/01/08 14:38:34, msarett wrote:
> nit: Line greater than 100 chars

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:316: : INHERITED(srcInfo, nullptr), fRawStream(stream),
fDngHost(host), fDngInfo(info), fDngNegative(negative) {}
On 2016/01/08 17:12:25, scroggo wrote:
> FWIW, in Skia we often will put each member on its own line, e.g.
> 
> SkRawCodec::SkRawCodec(...)
>     : INHERITED(srcInfo, nullptr)
>     , fRawStream(stream)
>     , ...

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.h#...
src/codec/SkRawCodec.h:59: * Called only by NewFromStream
On 2016/01/08 17:12:26, scroggo wrote:
> nit: Takes ownership of all of its pointer parameters

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStream.cpp#n...
src/core/SkStream.cpp:968: // TODO: optimize for the special case when the input
is SkMemoryStream.
On 2016/01/08 19:11:49, scroggo wrote:
> The optimization is actually elsewhere - you shouldn't need to call
SkStreamCopy
> if it's a memory stream (you should be able to just grab its SkData directly).

Not sure what you mean here, I moved the TODO into the SkRawCodec.cpp where this
function is called. I think we can still keep it as TODO, right?

https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/1520403003/diff/90001/src/core/SkStreamPriv.h...
src/core/SkStreamPriv.h:52: bool SkStreamCopy(SkDynamicMemoryWStream* out,
SkStream* input);
On 2016/01/08 19:11:49, scroggo wrote:
> Should this have a different name? Is it potentially problematic that
> SkStreamCopy has two implementations, where the parameters are the same except
> that one requires a subclass of the other?

Done.

scroggo

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode74 src/codec/SkRawCodec.cpp:74: dng_image* render_dng_for_prefered_size(const int prefered_size, dng_stream* stream, Can you add ...

4 years, 11 months ago (2016-01-11 17:44:03 UTC) #28

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:74: dng_image* render_dng_for_prefered_size(const int
prefered_size, dng_stream* stream,
Can you add comments explaining what this does, how the parameters are used, and
why it might fail?

A comment explaining what preferred_size means (the size of the longer side out
of W and H, after scaling) would also be nice.

Also, "prefered" is spelled incorrectly, here and elsewhere. It should be
"preferred".

One final nit: variables should be camelCase, so this parameter should be
"preferredSize" (unlike the name of the method, which is correctly
using_underscores).

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/11 14:03:07, yujieqin wrote:
> On 2016/01/08 14:38:34, msarett wrote:
> > I think here, you are assuming that getScaledDimensions() has already been
> > called, and then the client is calling getPixels() with the same size
returned
> > by getScaledDimensions()?
> > 
> > In that case, I think this will work.
> > 
> > However, the client is not required to use the size returned by
> > getScaledDimensions().  I think this will break if the client calls
> > getScaledDimensions(), but then decides to call getPixels() on a different
> size.
> > 
> > I'm in favor of not repeating the render_dng_for_prefered_size() call, but
to
> do
> > that, I think you'll need to make sure the current size of fDngImage matches
> the
> > request.
> 
> Done.

How was this fixed?

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:246: return kInvalidInput;
On 2016/01/11 14:03:07, yujieqin wrote:
> On 2016/01/08 17:12:26, scroggo wrote:
> > Arguably this could be kInvalidScale, although I don't know if you can tell
> the
> > difference.
> 
> We think "kInvalidInput" should be correct for this case, because the ckeck
for
> the size is done below.

So will render_dng_for_prefered_size succeed even if it's given a bad scale
value? (Or is there such thing? Will it always succeed if the input data is
valid?)

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:290: const int preferedSize = SkTMax(dim.fWidth,
dim.fHeight);
On 2016/01/11 14:03:07, yujieqin wrote:
> On 2016/01/08 14:38:34, msarett wrote:
> > Just for my curiosity, why does dng only need to know the max scaled
> dimension?
> 
> The DNG SDK only need the long edge size, because it always preservers the
> aspect ratio.

Good to know. Can you add a comment? Matt and I will need to maintain this code
in Skia, and that will help us understand what's going on the next time we look
at this code.

Something like:

// DNG SDK preserves the aspect ratio, so it only needs to know the longer
dimension.

https://codereview.chromium.org/1520403003/diff/110001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/110001/gyp/codec.gyp#newcode13
gyp/codec.gyp:13: 'common.gypi',
This is unnecessary. We actually include common.gypi in *all* our gyp files -
see gyp_skia, which takes care of that for you in python.

https://codereview.chromium.org/1520403003/diff/110001/gyp/common_conditions....
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/1520403003/diff/110001/gyp/common_conditions....
gyp/common_conditions.gypi:15: ['skia_codec_decodes_raw', {
This seems fine. I think you could have left this in the raw_codec target, but I
don't think this is a problem. (It just means that SK_CODEC_DECODES_RAW is
defined in targets that never know it or need to know it.)

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:132: return ::piex::Error::kFail;
nit: four space indent.

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:257: return kInvalidScale;
nit: four space indents

https://codereview.chromium.org/1520403003/diff/110001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/1520403003/diff/110001/src/core/SkStreamPriv....
src/core/SkStreamPriv.h:52: bool
SkStreamCopyToDynamicMemoryW(SkDynamicMemoryWStream* out, SkStream* input);
Reviewing this has forced me to take a long hard look at the methods in
SkStreamPriv.

I think we should move towards less versions of copying entire streams, rather
than more. (I've filed skbug.com/4788 to track this, which has some more
detail.)

To sum up: I don't think this function is necessary. I think you should call
SkStreamCopy from your code, and not mess with this file. (When I suggested
refactoring SkCopyStreamToStorage, I did not realize how many versions of the
same code we had.)

Taken a step further, you can just make the SkDynamicMemoryWStream your read and
write buffer. It is an SkWStream, so you can pass it to the existing
SkStreamCopy. It also has an interface that allows for reading. So rather than
copying it to a single contiguous block of memory, you can just call
SkDynamicMemoryWStream::read() in DoRead and GetData.

adaubert

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/11 17:44:02, scroggo wrote: > ...

4 years, 11 months ago (2016-01-11 17:57:05 UTC) #29

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/11 17:44:02, scroggo wrote:
> On 2016/01/11 14:03:07, yujieqin wrote:
> > On 2016/01/08 14:38:34, msarett wrote:
> > > I think here, you are assuming that getScaledDimensions() has already been
> > > called, and then the client is calling getPixels() with the same size
> returned
> > > by getScaledDimensions()?
> > > 
> > > In that case, I think this will work.
> > > 
> > > However, the client is not required to use the size returned by
> > > getScaledDimensions().  I think this will break if the client calls
> > > getScaledDimensions(), but then decides to call getPixels() on a different
> > size.
> > > 
> > > I'm in favor of not repeating the render_dng_for_prefered_size() call, but
> to
> > do
> > > that, I think you'll need to make sure the current size of fDngImage
matches
> > the
> > > request.
> > 
> > Done.
> 
> How was this fixed?

It was fixed in the way that the size of the image rendered by the
render_dng_for_prefered_size will be checked against the requested size. If the
sizes are different the error kInvalidScale will be returned.

* So if the caller called getScaledDimensions() before and then calls
onGetPixels() with exactly these size, it will work.
* If the caller called onGetPixels() directly, by requesting random sizes (so
not calling getScaledDimensions() before, or not calling  onGetPixels() with the
100% size), then the fDngImage will be created on the fly by trying to match the
requested size. If the requested size does not match the rendered size, then an
error will be returned.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:246: return kInvalidInput;
On 2016/01/11 17:44:02, scroggo wrote:
> On 2016/01/11 14:03:07, yujieqin wrote:
> > On 2016/01/08 17:12:26, scroggo wrote:
> > > Arguably this could be kInvalidScale, although I don't know if you can
tell
> > the
> > > difference.
> > 
> > We think "kInvalidInput" should be correct for this case, because the ckeck
> for
> > the size is done below.
> 
> So will render_dng_for_prefered_size succeed even if it's given a bad scale
> value? (Or is there such thing? Will it always succeed if the input data is
> valid?)

It will always succeed if the input data is valid.
The preferredSize is only a "wish" and the dng_sdk tries to make it, but it does
not guarantee that preferredSize will be delivered.

msarett

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/11 17:57:05, adaubert wrote: > ...

4 years, 11 months ago (2016-01-11 19:12:45 UTC) #30

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/11 17:57:05, adaubert wrote:
> On 2016/01/11 17:44:02, scroggo wrote:
> > On 2016/01/11 14:03:07, yujieqin wrote:
> > > On 2016/01/08 14:38:34, msarett wrote:
> > > > I think here, you are assuming that getScaledDimensions() has already
been
> > > > called, and then the client is calling getPixels() with the same size
> > returned
> > > > by getScaledDimensions()?
> > > > 
> > > > In that case, I think this will work.
> > > > 
> > > > However, the client is not required to use the size returned by
> > > > getScaledDimensions().  I think this will break if the client calls
> > > > getScaledDimensions(), but then decides to call getPixels() on a
different
> > > size.
> > > > 
> > > > I'm in favor of not repeating the render_dng_for_prefered_size() call,
but
> > to
> > > do
> > > > that, I think you'll need to make sure the current size of fDngImage
> matches
> > > the
> > > > request.
> > > 
> > > Done.
> > 
> > How was this fixed?
> 
> It was fixed in the way that the size of the image rendered by the
> render_dng_for_prefered_size will be checked against the requested size. If
the
> sizes are different the error kInvalidScale will be returned.
> 
> * So if the caller called getScaledDimensions() before and then calls
> onGetPixels() with exactly these size, it will work.
> * If the caller called onGetPixels() directly, by requesting random sizes (so
> not calling getScaledDimensions() before, or not calling  onGetPixels() with
the
> 100% size), then the fDngImage will be created on the fly by trying to match
the
> requested size. If the requested size does not match the rendered size, then
an
> error will be returned.

Ah I see.  This is not quite enough though.

After the client calls getScaledDimensions(), they should still be able to call
getPixels() on *any* valid size and get the correct result.

For example, maybe they don't like the recommended scaled dimensions, and decide
to just decode to the original size.  Or maybe they will call
getScaledDimensions() more than once, and then choose whichever size they like
best.

Essentially, onGetScaledDimensions() should behave as if it is truly const.  I
am noticing now that you have marked some fields as mutable.  I think this is
fine, as long as they are only used to cache the result of
onGetScaledDimensions() (in case it should match the request to onGetPixels()).

On the other hand, I would not say you *must* cache it.  I am fine with doing
the scaling calculation twice.

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:247: const int preferedSize = SkTMax(width, height);
I believe this should be:

preferredSize = originalWidth > originalHeight ? width : height

Otherwise, we might get tripped up if a bad client requests a width that is
larger than the height, when in reality the originalWidth is smaller than the
height.

yujieqin

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode74 src/codec/SkRawCodec.cpp:74: dng_image* render_dng_for_prefered_size(const int prefered_size, dng_stream* stream, On 2016/01/11 17:44:02, ...

4 years, 11 months ago (2016-01-12 10:47:41 UTC) #31

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:74: dng_image* render_dng_for_prefered_size(const int
prefered_size, dng_stream* stream,
On 2016/01/11 17:44:02, scroggo wrote:
> Can you add comments explaining what this does, how the parameters are used,
and
> why it might fail?
> 
> A comment explaining what preferred_size means (the size of the longer side
out
> of W and H, after scaling) would also be nice.
> 
> Also, "prefered" is spelled incorrectly, here and elsewhere. It should be
> "preferred".
> 
> One final nit: variables should be camelCase, so this parameter should be
> "preferredSize" (unlike the name of the method, which is correctly
> using_underscores).

Done.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/11 19:12:44, msarett wrote:
> On 2016/01/11 17:57:05, adaubert wrote:
> > On 2016/01/11 17:44:02, scroggo wrote:
> > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > I think here, you are assuming that getScaledDimensions() has already
> been
> > > > > called, and then the client is calling getPixels() with the same size
> > > returned
> > > > > by getScaledDimensions()?
> > > > > 
> > > > > In that case, I think this will work.
> > > > > 
> > > > > However, the client is not required to use the size returned by
> > > > > getScaledDimensions().  I think this will break if the client calls
> > > > > getScaledDimensions(), but then decides to call getPixels() on a
> different
> > > > size.
> > > > > 
> > > > > I'm in favor of not repeating the render_dng_for_prefered_size() call,
> but
> > > to
> > > > do
> > > > > that, I think you'll need to make sure the current size of fDngImage
> > matches
> > > > the
> > > > > request.
> > > > 
> > > > Done.
> > > 
> > > How was this fixed?
> > 
> > It was fixed in the way that the size of the image rendered by the
> > render_dng_for_prefered_size will be checked against the requested size. If
> the
> > sizes are different the error kInvalidScale will be returned.
> > 
> > * So if the caller called getScaledDimensions() before and then calls
> > onGetPixels() with exactly these size, it will work.
> > * If the caller called onGetPixels() directly, by requesting random sizes
(so
> > not calling getScaledDimensions() before, or not calling  onGetPixels() with
> the
> > 100% size), then the fDngImage will be created on the fly by trying to match
> the
> > requested size. If the requested size does not match the rendered size, then
> an
> > error will be returned.
> 
> Ah I see.  This is not quite enough though.
> 
> After the client calls getScaledDimensions(), they should still be able to
call
> getPixels() on *any* valid size and get the correct result.
> 
> For example, maybe they don't like the recommended scaled dimensions, and
decide
> to just decode to the original size.  Or maybe they will call
> getScaledDimensions() more than once, and then choose whichever size they like
> best.
> 
> Essentially, onGetScaledDimensions() should behave as if it is truly const.  I
> am noticing now that you have marked some fields as mutable.  I think this is
> fine, as long as they are only used to cache the result of
> onGetScaledDimensions() (in case it should match the request to
onGetPixels()).
> 
> On the other hand, I would not say you *must* cache it.  I am fine with doing
> the scaling calculation twice.

We prefer the caller use the size from onGetScaledDimensions(), because this
makes the process much faster (in such case, we only parse and render the image
once). Anyhow, we add the functionality to also support different size requests,
but this will cause multiple re-parsing and re-rendering.

Some more info:
The only reason to render the image already in onGetScaledDimensions() is: we
can only know the valid scaled image size after rendering. (due to the way the
preferred size is handled by DNG SDK.)

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:290: const int preferedSize = SkTMax(dim.fWidth,
dim.fHeight);
On 2016/01/11 17:44:02, scroggo wrote:
> On 2016/01/11 14:03:07, yujieqin wrote:
> > On 2016/01/08 14:38:34, msarett wrote:
> > > Just for my curiosity, why does dng only need to know the max scaled
> > dimension?
> > 
> > The DNG SDK only need the long edge size, because it always preservers the
> > aspect ratio.
> 
> Good to know. Can you add a comment? Matt and I will need to maintain this
code
> in Skia, and that will help us understand what's going on the next time we
look
> at this code.
> 
> Something like:
> 
> // DNG SDK preserves the aspect ratio, so it only needs to know the longer
> dimension.

Comment added.

FYI, for maintaining the RAW related code in skia (also the deps to DNG SDK and
PIEX), we also commit to do that.

https://codereview.chromium.org/1520403003/diff/110001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/110001/gyp/codec.gyp#newcode13
gyp/codec.gyp:13: 'common.gypi',
On 2016/01/11 17:44:02, scroggo wrote:
> This is unnecessary. We actually include common.gypi in *all* our gyp files -
> see gyp_skia, which takes care of that for you in python.

Done.

https://codereview.chromium.org/1520403003/diff/110001/gyp/common_conditions....
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/1520403003/diff/110001/gyp/common_conditions....
gyp/common_conditions.gypi:15: ['skia_codec_decodes_raw', {
On 2016/01/11 17:44:02, scroggo wrote:
> This seems fine. I think you could have left this in the raw_codec target, but
I
> don't think this is a problem. (It just means that SK_CODEC_DECODES_RAW is
> defined in targets that never know it or need to know it.)

Sure thing, I moved this back to the codec.gyp. We only need this in that very
special target.

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:132: return ::piex::Error::kFail;
On 2016/01/11 17:44:02, scroggo wrote:
> nit: four space indent.

Done.

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:247: const int preferedSize = SkTMax(width, height);
On 2016/01/11 19:12:44, msarett wrote:
> I believe this should be:
> 
> preferredSize = originalWidth > originalHeight ? width : height
> 
> Otherwise, we might get tripped up if a bad client requests a width that is
> larger than the height, when in reality the originalWidth is smaller than the
> height.

onGetPixel() should only support the requests, which take the size from
onGetScaledDimensions(). If the callers asked a portrait oriented size for a
landscape oriented image, we will return "kInvalidScale" error anyway. So we can
keep the SkTMax() as it is.

Or do you prefer the onGetPixel() to be more relax to error request, so that it
will always try to write something into the output (e.g. only write the center
part into result)?

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:257: return kInvalidScale;
On 2016/01/11 17:44:02, scroggo wrote:
> nit: four space indents

Done.

msarett

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/12 10:47:40, yujieqin wrote: > ...

4 years, 11 months ago (2016-01-12 14:01:29 UTC) #32

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/12 10:47:40, yujieqin wrote:
> On 2016/01/11 19:12:44, msarett wrote:
> > On 2016/01/11 17:57:05, adaubert wrote:
> > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > I think here, you are assuming that getScaledDimensions() has
already
> > been
> > > > > > called, and then the client is calling getPixels() with the same
size
> > > > returned
> > > > > > by getScaledDimensions()?
> > > > > > 
> > > > > > In that case, I think this will work.
> > > > > > 
> > > > > > However, the client is not required to use the size returned by
> > > > > > getScaledDimensions().  I think this will break if the client calls
> > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > different
> > > > > size.
> > > > > > 
> > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
call,
> > but
> > > > to
> > > > > do
> > > > > > that, I think you'll need to make sure the current size of fDngImage
> > > matches
> > > > > the
> > > > > > request.
> > > > > 
> > > > > Done.
> > > > 
> > > > How was this fixed?
> > > 
> > > It was fixed in the way that the size of the image rendered by the
> > > render_dng_for_prefered_size will be checked against the requested size.
If
> > the
> > > sizes are different the error kInvalidScale will be returned.
> > > 
> > > * So if the caller called getScaledDimensions() before and then calls
> > > onGetPixels() with exactly these size, it will work.
> > > * If the caller called onGetPixels() directly, by requesting random sizes
> (so
> > > not calling getScaledDimensions() before, or not calling  onGetPixels()
with
> > the
> > > 100% size), then the fDngImage will be created on the fly by trying to
match
> > the
> > > requested size. If the requested size does not match the rendered size,
then
> > an
> > > error will be returned.
> > 
> > Ah I see.  This is not quite enough though.
> > 
> > After the client calls getScaledDimensions(), they should still be able to
> call
> > getPixels() on *any* valid size and get the correct result.
> > 
> > For example, maybe they don't like the recommended scaled dimensions, and
> decide
> > to just decode to the original size.  Or maybe they will call
> > getScaledDimensions() more than once, and then choose whichever size they
like
> > best.
> > 
> > Essentially, onGetScaledDimensions() should behave as if it is truly const. 
I
> > am noticing now that you have marked some fields as mutable.  I think this
is
> > fine, as long as they are only used to cache the result of
> > onGetScaledDimensions() (in case it should match the request to
> onGetPixels()).
> > 
> > On the other hand, I would not say you *must* cache it.  I am fine with
doing
> > the scaling calculation twice.
> 
> We prefer the caller use the size from onGetScaledDimensions(), because this
> makes the process much faster (in such case, we only parse and render the
image
> once). Anyhow, we add the functionality to also support different size
requests,
> but this will cause multiple re-parsing and re-rendering.
> 
> Some more info:
> The only reason to render the image already in onGetScaledDimensions() is: we
> can only know the valid scaled image size after rendering. (due to the way the
> preferred size is handled by DNG SDK.)

Ok thanks I understand.  Maybe returning kInvalidInput is a good way to prevent
from the client from doing extra work?  What do you think Leon?

I think I would still prefer being more flexible but add warnings:
<on call to onGetPixels() with dimensions that don't match
onGetScaledDimensions()>
SkCodecPrintf("Warning: For RAW images, calling onGetPixels() with a size that
does not match the result of onGetScaledDimensions() forces the decoder to
render the image multiple times.")
(if the requested size is unsupported, this should still return kInvalidScale
later)

<on multiple calls to onGetScaledDimensions()>
SkCodecPrintf("Warning: Multiple calls to onGetScaledDimensions() for RAW image.
 Every call requires the image to be re-rendered.")

(no need to quote me exactly on these message if you have something more useful
to say :))

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/110001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:247: const int preferedSize = SkTMax(width, height);
On 2016/01/12 10:47:40, yujieqin wrote:
> On 2016/01/11 19:12:44, msarett wrote:
> > I believe this should be:
> > 
> > preferredSize = originalWidth > originalHeight ? width : height
> > 
> > Otherwise, we might get tripped up if a bad client requests a width that is
> > larger than the height, when in reality the originalWidth is smaller than
the
> > height.
> 
> onGetPixel() should only support the requests, which take the size from
> onGetScaledDimensions(). If the callers asked a portrait oriented size for a
> landscape oriented image, we will return "kInvalidScale" error anyway. So we
can
> keep the SkTMax() as it is.
> 
> Or do you prefer the onGetPixel() to be more relax to error request, so that
it
> will always try to write something into the output (e.g. only write the center
> part into result)?

My mistake you're right - if we ever pass the wrong dimension to
render_dng_for_preferred_size() (because the client requests a strange
orientation), we will still catch the error and return kInvalidScale below.

djsollen

djsollen@google.com changed reviewers: - djsollen@google.com

4 years, 11 months ago (2016-01-12 14:08:02 UTC) #33

yujieqin

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/12 14:01:29, msarett wrote: > ...

4 years, 11 months ago (2016-01-12 15:11:09 UTC) #34

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/12 14:01:29, msarett wrote:
> On 2016/01/12 10:47:40, yujieqin wrote:
> > On 2016/01/11 19:12:44, msarett wrote:
> > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > I think here, you are assuming that getScaledDimensions() has
> already
> > > been
> > > > > > > called, and then the client is calling getPixels() with the same
> size
> > > > > returned
> > > > > > > by getScaledDimensions()?
> > > > > > > 
> > > > > > > In that case, I think this will work.
> > > > > > > 
> > > > > > > However, the client is not required to use the size returned by
> > > > > > > getScaledDimensions().  I think this will break if the client
calls
> > > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > > different
> > > > > > size.
> > > > > > > 
> > > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
> call,
> > > but
> > > > > to
> > > > > > do
> > > > > > > that, I think you'll need to make sure the current size of
fDngImage
> > > > matches
> > > > > > the
> > > > > > > request.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > How was this fixed?
> > > > 
> > > > It was fixed in the way that the size of the image rendered by the
> > > > render_dng_for_prefered_size will be checked against the requested size.
> If
> > > the
> > > > sizes are different the error kInvalidScale will be returned.
> > > > 
> > > > * So if the caller called getScaledDimensions() before and then calls
> > > > onGetPixels() with exactly these size, it will work.
> > > > * If the caller called onGetPixels() directly, by requesting random
sizes
> > (so
> > > > not calling getScaledDimensions() before, or not calling  onGetPixels()
> with
> > > the
> > > > 100% size), then the fDngImage will be created on the fly by trying to
> match
> > > the
> > > > requested size. If the requested size does not match the rendered size,
> then
> > > an
> > > > error will be returned.
> > > 
> > > Ah I see.  This is not quite enough though.
> > > 
> > > After the client calls getScaledDimensions(), they should still be able to
> > call
> > > getPixels() on *any* valid size and get the correct result.
> > > 
> > > For example, maybe they don't like the recommended scaled dimensions, and
> > decide
> > > to just decode to the original size.  Or maybe they will call
> > > getScaledDimensions() more than once, and then choose whichever size they
> like
> > > best.
> > > 
> > > Essentially, onGetScaledDimensions() should behave as if it is truly
const. 
> I
> > > am noticing now that you have marked some fields as mutable.  I think this
> is
> > > fine, as long as they are only used to cache the result of
> > > onGetScaledDimensions() (in case it should match the request to
> > onGetPixels()).
> > > 
> > > On the other hand, I would not say you *must* cache it.  I am fine with
> doing
> > > the scaling calculation twice.
> > 
> > We prefer the caller use the size from onGetScaledDimensions(), because this
> > makes the process much faster (in such case, we only parse and render the
> image
> > once). Anyhow, we add the functionality to also support different size
> requests,
> > but this will cause multiple re-parsing and re-rendering.
> > 
> > Some more info:
> > The only reason to render the image already in onGetScaledDimensions() is:
we
> > can only know the valid scaled image size after rendering. (due to the way
the
> > preferred size is handled by DNG SDK.)
> 
> Ok thanks I understand.  Maybe returning kInvalidInput is a good way to
prevent
> from the client from doing extra work?  What do you think Leon?
> 
> I think I would still prefer being more flexible but add warnings:
> <on call to onGetPixels() with dimensions that don't match
> onGetScaledDimensions()>
> SkCodecPrintf("Warning: For RAW images, calling onGetPixels() with a size that
> does not match the result of onGetScaledDimensions() forces the decoder to
> render the image multiple times.")
> (if the requested size is unsupported, this should still return kInvalidScale
> later)
> 
> <on multiple calls to onGetScaledDimensions()>
> SkCodecPrintf("Warning: Multiple calls to onGetScaledDimensions() for RAW
image.
>  Every call requires the image to be re-rendered.")
> 
> (no need to quote me exactly on these message if you have something more
useful
> to say :))

Added the warnings.

scroggo

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/12 10:47:40, yujieqin wrote: > ...

4 years, 11 months ago (2016-01-12 21:54:39 UTC) #36

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/12 10:47:40, yujieqin wrote:
> On 2016/01/11 19:12:44, msarett wrote:
> > On 2016/01/11 17:57:05, adaubert wrote:
> > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > I think here, you are assuming that getScaledDimensions() has
already
> > been
> > > > > > called, and then the client is calling getPixels() with the same
size
> > > > returned
> > > > > > by getScaledDimensions()?
> > > > > > 
> > > > > > In that case, I think this will work.
> > > > > > 
> > > > > > However, the client is not required to use the size returned by
> > > > > > getScaledDimensions().  I think this will break if the client calls
> > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > different
> > > > > size.
> > > > > > 
> > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
call,
> > but
> > > > to
> > > > > do
> > > > > > that, I think you'll need to make sure the current size of fDngImage
> > > matches
> > > > > the
> > > > > > request.
> > > > > 
> > > > > Done.
> > > > 
> > > > How was this fixed?
> > > 
> > > It was fixed in the way that the size of the image rendered by the
> > > render_dng_for_prefered_size will be checked against the requested size.
If
> > the
> > > sizes are different the error kInvalidScale will be returned.
> > > 
> > > * So if the caller called getScaledDimensions() before and then calls
> > > onGetPixels() with exactly these size, it will work.
> > > * If the caller called onGetPixels() directly, by requesting random sizes
> (so
> > > not calling getScaledDimensions() before, or not calling  onGetPixels()
with
> > the
> > > 100% size), then the fDngImage will be created on the fly by trying to
match
> > the
> > > requested size. If the requested size does not match the rendered size,
then
> > an
> > > error will be returned.
> > 
> > Ah I see.  This is not quite enough though.
> > 
> > After the client calls getScaledDimensions(), they should still be able to
> call
> > getPixels() on *any* valid size and get the correct result.
> > 
> > For example, maybe they don't like the recommended scaled dimensions, and
> decide
> > to just decode to the original size.  Or maybe they will call
> > getScaledDimensions() more than once, and then choose whichever size they
like
> > best.
> > 
> > Essentially, onGetScaledDimensions() should behave as if it is truly const. 
I
> > am noticing now that you have marked some fields as mutable.  I think this
is
> > fine, as long as they are only used to cache the result of
> > onGetScaledDimensions() (in case it should match the request to
> onGetPixels()).
> > 
> > On the other hand, I would not say you *must* cache it.  I am fine with
doing
> > the scaling calculation twice.
> 
> We prefer the caller use the size from onGetScaledDimensions(), because this
> makes the process much faster

Right, but this is a virtual function, and other SkCodecs expect to be able to
check a couple of times, and potentially use a different scale. FWIW,
SkImageGenerator built on the SkCodec API, and has the method
computeScaledDimensions (introduced in crrev.com/1396323007) which returns up to
two options for supported dimensions. It is possible we'll convert SkCodec's API
to match this one. If we cannot update the underlying libraries to do the check
more efficiently (is that not possible?), I suppose we'll just have to return
one possible set of supported dimensions, in order to run faster and throw away
less work. Do you have a rough idea of how long it takes to call
getScaledDimensions? 

> (in such case, we only parse and render the image
> once). Anyhow, we add the functionality to also support different size
requests,
> but this will cause multiple re-parsing and re-rendering.

So this means that the fully decoded raw image is stored in the SkCodec? How big
is a typical DngImage? SkCodec was designed around the idea that it could be
used for a "lazy" decode - we only decode enough initially to know the size, and
then when the client wants the actual image (e.g. at draw time, or in a
background thread) they can call getPixels. But this means that we may keep
around the DngImage at times when we do not need it.

> 
> Some more info:
> The only reason to render the image already in onGetScaledDimensions() is: we
> can only know the valid scaled image size after rendering. (due to the way the
> preferred size is handled by DNG SDK.)

Did your team also write the DNG SDK? Is it possible to change that restriction?

I think with the current implementation, we should also put a note on
getScaledDimensions, that it is an expensive call (which holds on to a lot of
RAM) for these images.

https://codereview.chromium.org/1520403003/diff/170001/DEPS
File DEPS (right):

https://codereview.chromium.org/1520403003/diff/170001/DEPS#newcode26
DEPS:26: "third_party/externals/piex"     :
"https://skia.googlesource.com/third_party/piex/",
Why does this use a mirror? Isn't this also checked into Android? Can we use the
android.googlesource.com version directly?

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:39: if (sum <= std::numeric_limits<size_t>::max()) {
How can this be false if the above if statement is true?

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:71: * Renders the DNG image, which is loaded by
read_dng().
I find this comment confusing. Is it saying that it calls read_dng? Or that
read_dng already loaded the image? But it looks like read_dng created a
"negative", not an "image". I think a clearer comment might say something like:

Renders the DNG image to the preferred size.

If you want to say something about read_dng, I think it probably makes sense to
specify it along with the parameter descriptions. e.g.

host: already initialized by read_dng
info: already initialized by read_dng
negative: loaded by read_dng

(That said, down below, I suggested making this method a member method, in which
case you do not need to pass the parameters in.)

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:74: * Determines the preferred size of the long edge.
The rendered image will be close to this size,
nit: Specifies the preferred size ...

sounds better to me.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:81: * Contains the data of the image.
I think this is more clear as: Contains the compressed data representing the
image.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:139: SkMemoryStream* copyBuffer(size_t offset, size_t
size) {
I think this method could be more efficient. Assuming this is called before we
reach |offset| in |fStream|, it is going to buffer:

a) the stream from its current offset until |offset|
b) the stream from |offset| to |offset| + |size|

You don't actually care about (a) (maybe its zero in practice, but then it seems
like we don't need the parameter |offset|), and (b) is just going to be copied
again. You never need the first copy.

I think what you really want is something like the following:

if (!fStream->skip(offset - fStreamBuffer.bytesWritten()) {
    return nullptr;
}

// Note that there is a TODO in SkStream to eliminate getMemoryBase
// This is how you use an SkData to do the same thing:
SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
if (fStream->read(data->writable_data(), size) != size) {
    return nullptr;
}

return new SkMemoryStream(data);

Two caveats:
- This is destructive to the SkRawStream. In practice, I think this is fine
because you never use it after calling copyBuffer. But I do think it is worth a
comment.
- I'm assuming that you have not already buffered past offset. If you have, you
can read fStreamBuffer into the new SkData, and then copy any remaining bytes
directly into it. This will be only slightly more complicated.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:204: if (newSize <= fStreamBuffer.bytesWritten()) {  //
the stream is already buffered to newSize.
nit: 100 chars

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:213: tempBuffer.realloc(sizeToRead);
nit: You can combine these two lines into:

SkAutoTMalloc<uint8> tempBuffer(sizeToRead);

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:237: return memoryStream ?
SkJpegCodec::NewFromStream(memoryStream) : nullptr;
We'll need to skip the JPEG code ing Google3 (i.e. "#if !defined(GOOGLE3)", as
in SkCodec.cpp), unless it turns out we cannot build SkRawCodec at all in
Google3 (I don't know its restrictions; I don't remember why SkJpegCodec is not
allowed there).

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:247: const SkImageInfo& imageInfo = SkImageInfo::Make(
I don't think this is right to make this a reference. SkImageInfo::Make will
return a copy, unless it's inlined, in which case you're essentially calling the
constructor. Either way, this should just be "const SkImageInfo"

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:270: fDngHost.reset(new dng_host);
Can these fields not be reused?

I also notice that the SkRawCodec never cares about the dng_host, dng_info, or
dng_negative. It only hangs on to them to pass them to static functions. Would
it make sense to abstract this into its own class?

class SkDngImage {
private:
    SkDngImage(...); // constructor
    fDngImage;
    fDngInfo;
    fDngNegative;
    fDngHost;
    fRawStream;
public:
    static SkDngImage* NewFromStream(SkRawStream*) {
        // returns nullptr when read_dng would have failed
    }

    bool /* or SkCodec::Result? */ scale(int width, int height) {
        int prefSize = SkTMax(width, height);
        // does what render_dng_for_preferred_size does now
        if (!fDngImage) {
            return false; // or kInvalidInput;
        }
        // Possibly check for matching width/height here...
    }
}

Alternatively, should read_dng and render_dng_for_preferred_size be private
member functions on SkRawCodec? Then we do not have to pass all these pointers
around all the time.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:322: if (desiredScale >= 1.0) {
This is redundant. We already call this in getScaledDimensions (which calls this
function - this function should not otherwise be called directly).

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:353: return dim.width() >= 1 && dim.width() <=
info.width()
This is misleading. It reports that it can support any arbitrary scale, but this
is not true. That said, the function was really put in place to early exit if
the codec knows it cannot support this scale. In your case, it seems that the
codec does not know. Perhaps we should update the header to state that it
returns false positives?

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.h...
src/codec/SkRawCodec.h:42: * Supports only requests, which take the size from
onGetScaledDimensions().
I thought you had decided to support other requests?

If this comment is necessary, though, it needs to also be in SkCodec.h on
getScaledDimensions. This is a virtual function, so a client might be calling it
on a pointer to SkCodec.

Also, nit: Unnecessary comma.

yujieqin

We change the logic for scaling DNG image. So it should fit better to skia ...

4 years, 11 months ago (2016-01-13 15:29:42 UTC) #37

We change the logic for scaling DNG image. So it should fit better to skia
requirements.

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/12 21:54:39, scroggo wrote:
> On 2016/01/12 10:47:40, yujieqin wrote:
> > On 2016/01/11 19:12:44, msarett wrote:
> > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > I think here, you are assuming that getScaledDimensions() has
> already
> > > been
> > > > > > > called, and then the client is calling getPixels() with the same
> size
> > > > > returned
> > > > > > > by getScaledDimensions()?
> > > > > > > 
> > > > > > > In that case, I think this will work.
> > > > > > > 
> > > > > > > However, the client is not required to use the size returned by
> > > > > > > getScaledDimensions().  I think this will break if the client
calls
> > > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > > different
> > > > > > size.
> > > > > > > 
> > > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
> call,
> > > but
> > > > > to
> > > > > > do
> > > > > > > that, I think you'll need to make sure the current size of
fDngImage
> > > > matches
> > > > > > the
> > > > > > > request.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > How was this fixed?
> > > > 
> > > > It was fixed in the way that the size of the image rendered by the
> > > > render_dng_for_prefered_size will be checked against the requested size.
> If
> > > the
> > > > sizes are different the error kInvalidScale will be returned.
> > > > 
> > > > * So if the caller called getScaledDimensions() before and then calls
> > > > onGetPixels() with exactly these size, it will work.
> > > > * If the caller called onGetPixels() directly, by requesting random
sizes
> > (so
> > > > not calling getScaledDimensions() before, or not calling  onGetPixels()
> with
> > > the
> > > > 100% size), then the fDngImage will be created on the fly by trying to
> match
> > > the
> > > > requested size. If the requested size does not match the rendered size,
> then
> > > an
> > > > error will be returned.
> > > 
> > > Ah I see.  This is not quite enough though.
> > > 
> > > After the client calls getScaledDimensions(), they should still be able to
> > call
> > > getPixels() on *any* valid size and get the correct result.
> > > 
> > > For example, maybe they don't like the recommended scaled dimensions, and
> > decide
> > > to just decode to the original size.  Or maybe they will call
> > > getScaledDimensions() more than once, and then choose whichever size they
> like
> > > best.
> > > 
> > > Essentially, onGetScaledDimensions() should behave as if it is truly
const. 
> I
> > > am noticing now that you have marked some fields as mutable.  I think this
> is
> > > fine, as long as they are only used to cache the result of
> > > onGetScaledDimensions() (in case it should match the request to
> > onGetPixels()).
> > > 
> > > On the other hand, I would not say you *must* cache it.  I am fine with
> doing
> > > the scaling calculation twice.
> > 
> > We prefer the caller use the size from onGetScaledDimensions(), because this
> > makes the process much faster
> 
> Right, but this is a virtual function, and other SkCodecs expect to be able to
> check a couple of times, and potentially use a different scale. FWIW,
> SkImageGenerator built on the SkCodec API, and has the method
> computeScaledDimensions (introduced in crrev.com/1396323007) which returns up
to
> two options for supported dimensions. It is possible we'll convert SkCodec's
API
> to match this one. If we cannot update the underlying libraries to do the
check
> more efficiently (is that not possible?), I suppose we'll just have to return
> one possible set of supported dimensions, in order to run faster and throw
away
> less work. Do you have a rough idea of how long it takes to call
> getScaledDimensions? 
> 
> > (in such case, we only parse and render the image
> > once). Anyhow, we add the functionality to also support different size
> requests,
> > but this will cause multiple re-parsing and re-rendering.
> 
> So this means that the fully decoded raw image is stored in the SkCodec? How
big
> is a typical DngImage? SkCodec was designed around the idea that it could be
> used for a "lazy" decode - we only decode enough initially to know the size,
and
> then when the client wants the actual image (e.g. at draw time, or in a
> background thread) they can call getPixels. But this means that we may keep
> around the DngImage at times when we do not need it.
> 
> > 
> > Some more info:
> > The only reason to render the image already in onGetScaledDimensions() is:
we
> > can only know the valid scaled image size after rendering. (due to the way
the
> > preferred size is handled by DNG SDK.)
> 
> Did your team also write the DNG SDK? Is it possible to change that
restriction?
> 
> I think with the current implementation, we should also put a note on
> getScaledDimensions, that it is an expensive call (which holds on to a lot of
> RAM) for these images.

We refactor the whole logic of scaling DNG images. We now do cheap computation
for onGetScaledDimensions() and made the onDimensionsSupported() more reliable.
The heavy work is now done only in onGEtPixels(). This change also implies the
rendering of DNG image is done "lazy", only in onGetPixels().

The DNG SDK is from Adobe. We think our new solution for handling scale request
should be okay.

Will the onGetPixels() be called multiple time with the same resolution for our
use case? If not, we can even clean up all the DNG stuff (free up memory) except
the rawStream and recreate everything from the rawStream if onGetPixels() get
called again.

https://codereview.chromium.org/1520403003/diff/110001/src/core/SkStreamPriv.h
File src/core/SkStreamPriv.h (right):

https://codereview.chromium.org/1520403003/diff/110001/src/core/SkStreamPriv....
src/core/SkStreamPriv.h:52: bool
SkStreamCopyToDynamicMemoryW(SkDynamicMemoryWStream* out, SkStream* input);
On 2016/01/11 17:44:02, scroggo wrote:
> Reviewing this has forced me to take a long hard look at the methods in
> SkStreamPriv.
> 
> I think we should move towards less versions of copying entire streams, rather
> than more. (I've filed skbug.com/4788 to track this, which has some more
> detail.)
> 
> To sum up: I don't think this function is necessary. I think you should call
> SkStreamCopy from your code, and not mess with this file. (When I suggested
> refactoring SkCopyStreamToStorage, I did not realize how many versions of the
> same code we had.)
> 
> Taken a step further, you can just make the SkDynamicMemoryWStream your read
and
> write buffer. It is an SkWStream, so you can pass it to the existing
> SkStreamCopy. It also has an interface that allows for reading. So rather than
> copying it to a single contiguous block of memory, you can just call
> SkDynamicMemoryWStream::read() in DoRead and GetData.

reverted.

https://codereview.chromium.org/1520403003/diff/170001/DEPS
File DEPS (right):

https://codereview.chromium.org/1520403003/diff/170001/DEPS#newcode26
DEPS:26: "third_party/externals/piex"     :
"https://skia.googlesource.com/third_party/piex/",
On 2016/01/12 21:54:39, scroggo wrote:
> Why does this use a mirror? Isn't this also checked into Android? Can we use
the
> http://android.googlesource.com version directly?

We are in the process of getting the repository in AOSP for PIEX. Please see
b/26146381 (just CC-ed you). It seems like we got an internal repository,
jaesung@ is trying to find out if we can get a external ASOP/PIEX repository
(similar to DNG SDK).

Just a quick question, would it be an issue for current CL if the AOSP/PIEX
repository is internal?

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:39: if (sum <= std::numeric_limits<size_t>::max()) {
On 2016/01/12 21:54:39, scroggo wrote:
> How can this be false if the above if statement is true?

T could be uint64 in our use case, so we need this check.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:71: * Renders the DNG image, which is loaded by
read_dng().
On 2016/01/12 21:54:39, scroggo wrote:
> I find this comment confusing. Is it saying that it calls read_dng? Or that
> read_dng already loaded the image? But it looks like read_dng created a
> "negative", not an "image". I think a clearer comment might say something
like:
> 
> Renders the DNG image to the preferred size.
> 
> If you want to say something about read_dng, I think it probably makes sense
to
> specify it along with the parameter descriptions. e.g.
> 
> host: already initialized by read_dng
> info: already initialized by read_dng
> negative: loaded by read_dng
> 
> (That said, down below, I suggested making this method a member method, in
which
> case you do not need to pass the parameters in.)

moved into the class SkDngImage, then we don't need the old comments anymore.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:74: * Determines the preferred size of the long edge.
The rendered image will be close to this size,
On 2016/01/12 21:54:39, scroggo wrote:
> nit: Specifies the preferred size ...
> 
> sounds better to me.

Done.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:81: * Contains the data of the image.
On 2016/01/12 21:54:39, scroggo wrote:
> I think this is more clear as: Contains the compressed data representing the
> image.

Comment removed.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:139: SkMemoryStream* copyBuffer(size_t offset, size_t
size) {
On 2016/01/12 21:54:39, scroggo wrote:
> I think this method could be more efficient. Assuming this is called before we
> reach |offset| in |fStream|, it is going to buffer:
> 
> a) the stream from its current offset until |offset|
> b) the stream from |offset| to |offset| + |size|
> 
> You don't actually care about (a) (maybe its zero in practice, but then it
seems
> like we don't need the parameter |offset|), and (b) is just going to be copied
> again. You never need the first copy.
> 
> I think what you really want is something like the following:
> 
> if (!fStream->skip(offset - fStreamBuffer.bytesWritten()) {
>     return nullptr;
> }
> 
> // Note that there is a TODO in SkStream to eliminate getMemoryBase
> // This is how you use an SkData to do the same thing:
> SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
> if (fStream->read(data->writable_data(), size) != size) {
>     return nullptr;
> }
> 
> return new SkMemoryStream(data);
> 
> Two caveats:
> - This is destructive to the SkRawStream. In practice, I think this is fine
> because you never use it after calling copyBuffer. But I do think it is worth
a
> comment.
> - I'm assuming that you have not already buffered past offset. If you have,
you
> can read fStreamBuffer into the new SkData, and then copy any remaining bytes
> directly into it. This will be only slightly more complicated.

Agreed, I think it is a good improvement if the offset is not buffered yet. (in
most of the case, this can be actually true).

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:204: if (newSize <= fStreamBuffer.bytesWritten()) {  //
the stream is already buffered to newSize.
On 2016/01/12 21:54:39, scroggo wrote:
> nit: 100 chars

Done.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:213: tempBuffer.realloc(sizeToRead);
On 2016/01/12 21:54:39, scroggo wrote:
> nit: You can combine these two lines into:
> 
> SkAutoTMalloc<uint8> tempBuffer(sizeToRead);

Thanks.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:237: return memoryStream ?
SkJpegCodec::NewFromStream(memoryStream) : nullptr;
On 2016/01/12 21:54:39, scroggo wrote:
> We'll need to skip the JPEG code ing Google3 (i.e. "#if !defined(GOOGLE3)", as
> in SkCodec.cpp), unless it turns out we cannot build SkRawCodec at all in
> Google3 (I don't know its restrictions; I don't remember why SkJpegCodec is
not
> allowed there).

Done

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:247: const SkImageInfo& imageInfo = SkImageInfo::Make(
On 2016/01/12 21:54:39, scroggo wrote:
> I don't think this is right to make this a reference. SkImageInfo::Make will
> return a copy, unless it's inlined, in which case you're essentially calling
the
> constructor. Either way, this should just be "const SkImageInfo"

Removed.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:270: fDngHost.reset(new dng_host);
On 2016/01/12 21:54:39, scroggo wrote:
> Can these fields not be reused?
> 
> I also notice that the SkRawCodec never cares about the dng_host, dng_info, or
> dng_negative. It only hangs on to them to pass them to static functions. Would
> it make sense to abstract this into its own class?
> 
> class SkDngImage {
> private:
>     SkDngImage(...); // constructor
>     fDngImage;
>     fDngInfo;
>     fDngNegative;
>     fDngHost;
>     fRawStream;
> public:
>     static SkDngImage* NewFromStream(SkRawStream*) {
>         // returns nullptr when read_dng would have failed
>     }
> 
>     bool /* or SkCodec::Result? */ scale(int width, int height) {
>         int prefSize = SkTMax(width, height);
>         // does what render_dng_for_preferred_size does now
>         if (!fDngImage) {
>             return false; // or kInvalidInput;
>         }
>         // Possibly check for matching width/height here...
>     }
> }
> 
> Alternatively, should read_dng and render_dng_for_preferred_size be private
> member functions on SkRawCodec? Then we do not have to pass all these pointers
> around all the time.

Done.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:322: if (desiredScale >= 1.0) {
On 2016/01/12 21:54:39, scroggo wrote:
> This is redundant. We already call this in getScaledDimensions (which calls
this
> function - this function should not otherwise be called directly).

Done.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:353: return dim.width() >= 1 && dim.width() <=
info.width()
On 2016/01/12 21:54:39, scroggo wrote:
> This is misleading. It reports that it can support any arbitrary scale, but
this
> is not true. That said, the function was really put in place to early exit if
> the codec knows it cannot support this scale. In your case, it seems that the
> codec does not know. Perhaps we should update the header to state that it
> returns false positives?

We now offer a more reliable check.

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/170001/src/codec/SkRawCodec.h...
src/codec/SkRawCodec.h:42: * Supports only requests, which take the size from
onGetScaledDimensions().
On 2016/01/12 21:54:39, scroggo wrote:
> I thought you had decided to support other requests?
> 
> If this comment is necessary, though, it needs to also be in SkCodec.h on
> getScaledDimensions. This is a virtual function, so a client might be calling
it
> on a pointer to SkCodec.
> 
> Also, nit: Unnecessary comma.
> 

You are right, it is misleading. Removed.

msarett

I think the new implementations of onGetScaledDimensions() and onDimensionsSupported() are a great step! https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File ...

4 years, 11 months ago (2016-01-13 17:15:14 UTC) #38

I think the new implementations of onGetScaledDimensions() and
onDimensionsSupported() are a great step!

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/13 15:29:41, yujieqin wrote:
> On 2016/01/12 21:54:39, scroggo wrote:
> > On 2016/01/12 10:47:40, yujieqin wrote:
> > > On 2016/01/11 19:12:44, msarett wrote:
> > > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > > I think here, you are assuming that getScaledDimensions() has
> > already
> > > > been
> > > > > > > > called, and then the client is calling getPixels() with the same
> > size
> > > > > > returned
> > > > > > > > by getScaledDimensions()?
> > > > > > > > 
> > > > > > > > In that case, I think this will work.
> > > > > > > > 
> > > > > > > > However, the client is not required to use the size returned by
> > > > > > > > getScaledDimensions().  I think this will break if the client
> calls
> > > > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > > > different
> > > > > > > size.
> > > > > > > > 
> > > > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
> > call,
> > > > but
> > > > > > to
> > > > > > > do
> > > > > > > > that, I think you'll need to make sure the current size of
> fDngImage
> > > > > matches
> > > > > > > the
> > > > > > > > request.
> > > > > > > 
> > > > > > > Done.
> > > > > > 
> > > > > > How was this fixed?
> > > > > 
> > > > > It was fixed in the way that the size of the image rendered by the
> > > > > render_dng_for_prefered_size will be checked against the requested
size.
> > If
> > > > the
> > > > > sizes are different the error kInvalidScale will be returned.
> > > > > 
> > > > > * So if the caller called getScaledDimensions() before and then calls
> > > > > onGetPixels() with exactly these size, it will work.
> > > > > * If the caller called onGetPixels() directly, by requesting random
> sizes
> > > (so
> > > > > not calling getScaledDimensions() before, or not calling 
onGetPixels()
> > with
> > > > the
> > > > > 100% size), then the fDngImage will be created on the fly by trying to
> > match
> > > > the
> > > > > requested size. If the requested size does not match the rendered
size,
> > then
> > > > an
> > > > > error will be returned.
> > > > 
> > > > Ah I see.  This is not quite enough though.
> > > > 
> > > > After the client calls getScaledDimensions(), they should still be able
to
> > > call
> > > > getPixels() on *any* valid size and get the correct result.
> > > > 
> > > > For example, maybe they don't like the recommended scaled dimensions,
and
> > > decide
> > > > to just decode to the original size.  Or maybe they will call
> > > > getScaledDimensions() more than once, and then choose whichever size
they
> > like
> > > > best.
> > > > 
> > > > Essentially, onGetScaledDimensions() should behave as if it is truly
> const. 
> > I
> > > > am noticing now that you have marked some fields as mutable.  I think
this
> > is
> > > > fine, as long as they are only used to cache the result of
> > > > onGetScaledDimensions() (in case it should match the request to
> > > onGetPixels()).
> > > > 
> > > > On the other hand, I would not say you *must* cache it.  I am fine with
> > doing
> > > > the scaling calculation twice.
> > > 
> > > We prefer the caller use the size from onGetScaledDimensions(), because
this
> > > makes the process much faster
> > 
> > Right, but this is a virtual function, and other SkCodecs expect to be able
to
> > check a couple of times, and potentially use a different scale. FWIW,
> > SkImageGenerator built on the SkCodec API, and has the method
> > computeScaledDimensions (introduced in crrev.com/1396323007) which returns
up
> to
> > two options for supported dimensions. It is possible we'll convert SkCodec's
> API
> > to match this one. If we cannot update the underlying libraries to do the
> check
> > more efficiently (is that not possible?), I suppose we'll just have to
return
> > one possible set of supported dimensions, in order to run faster and throw
> away
> > less work. Do you have a rough idea of how long it takes to call
> > getScaledDimensions? 
> > 
> > > (in such case, we only parse and render the image
> > > once). Anyhow, we add the functionality to also support different size
> > requests,
> > > but this will cause multiple re-parsing and re-rendering.
> > 
> > So this means that the fully decoded raw image is stored in the SkCodec? How
> big
> > is a typical DngImage? SkCodec was designed around the idea that it could be
> > used for a "lazy" decode - we only decode enough initially to know the size,
> and
> > then when the client wants the actual image (e.g. at draw time, or in a
> > background thread) they can call getPixels. But this means that we may keep
> > around the DngImage at times when we do not need it.
> > 
> > > 
> > > Some more info:
> > > The only reason to render the image already in onGetScaledDimensions() is:
> we
> > > can only know the valid scaled image size after rendering. (due to the way
> the
> > > preferred size is handled by DNG SDK.)
> > 
> > Did your team also write the DNG SDK? Is it possible to change that
> restriction?
> > 
> > I think with the current implementation, we should also put a note on
> > getScaledDimensions, that it is an expensive call (which holds on to a lot
of
> > RAM) for these images.
> 
> We refactor the whole logic of scaling DNG images. We now do cheap computation
> for onGetScaledDimensions() and made the onDimensionsSupported() more
reliable.
> The heavy work is now done only in onGEtPixels(). This change also implies the
> rendering of DNG image is done "lazy", only in onGetPixels().
> 
> The DNG SDK is from Adobe. We think our new solution for handling scale
request
> should be okay.
> 
> Will the onGetPixels() be called multiple time with the same resolution for
our
> use case? If not, we can even clean up all the DNG stuff (free up memory)
except
> the rawStream and recreate everything from the rawStream if onGetPixels() get
> called again.

Good question.  I believe it might.  Certainly we have designed to make that use
case possible, but I'm not sure how frequent it is.  Leon may know more.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:408: // Limits the minimun size to be 128 on the long
edge.
This limitation seems reasonable.  Is this a limit imposed by the DNG lib or
arbitrarily by us?

My concern is that this is a different behavior than the other Codecs.

On the other hand, will RAW support all integer downscales (1, 2, 3, 5, 6 ...)? 
Because if not, we already have a difference in behavior anyway.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:422: return dim.width() >= 1 && dim.width() <=
info.width()
Everything below this line is dead code?

scroggo

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/13 15:29:41, yujieqin wrote: > ...

4 years, 11 months ago (2016-01-13 18:24:45 UTC) #39

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/13 15:29:41, yujieqin wrote:
> On 2016/01/12 21:54:39, scroggo wrote:
> > On 2016/01/12 10:47:40, yujieqin wrote:
> > > On 2016/01/11 19:12:44, msarett wrote:
> > > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > > I think here, you are assuming that getScaledDimensions() has
> > already
> > > > been
> > > > > > > > called, and then the client is calling getPixels() with the same
> > size
> > > > > > returned
> > > > > > > > by getScaledDimensions()?
> > > > > > > > 
> > > > > > > > In that case, I think this will work.
> > > > > > > > 
> > > > > > > > However, the client is not required to use the size returned by
> > > > > > > > getScaledDimensions().  I think this will break if the client
> calls
> > > > > > > > getScaledDimensions(), but then decides to call getPixels() on a
> > > > different
> > > > > > > size.
> > > > > > > > 
> > > > > > > > I'm in favor of not repeating the render_dng_for_prefered_size()
> > call,
> > > > but
> > > > > > to
> > > > > > > do
> > > > > > > > that, I think you'll need to make sure the current size of
> fDngImage
> > > > > matches
> > > > > > > the
> > > > > > > > request.
> > > > > > > 
> > > > > > > Done.
> > > > > > 
> > > > > > How was this fixed?
> > > > > 
> > > > > It was fixed in the way that the size of the image rendered by the
> > > > > render_dng_for_prefered_size will be checked against the requested
size.
> > If
> > > > the
> > > > > sizes are different the error kInvalidScale will be returned.
> > > > > 
> > > > > * So if the caller called getScaledDimensions() before and then calls
> > > > > onGetPixels() with exactly these size, it will work.
> > > > > * If the caller called onGetPixels() directly, by requesting random
> sizes
> > > (so
> > > > > not calling getScaledDimensions() before, or not calling 
onGetPixels()
> > with
> > > > the
> > > > > 100% size), then the fDngImage will be created on the fly by trying to
> > match
> > > > the
> > > > > requested size. If the requested size does not match the rendered
size,
> > then
> > > > an
> > > > > error will be returned.
> > > > 
> > > > Ah I see.  This is not quite enough though.
> > > > 
> > > > After the client calls getScaledDimensions(), they should still be able
to
> > > call
> > > > getPixels() on *any* valid size and get the correct result.
> > > > 
> > > > For example, maybe they don't like the recommended scaled dimensions,
and
> > > decide
> > > > to just decode to the original size.  Or maybe they will call
> > > > getScaledDimensions() more than once, and then choose whichever size
they
> > like
> > > > best.
> > > > 
> > > > Essentially, onGetScaledDimensions() should behave as if it is truly
> const. 
> > I
> > > > am noticing now that you have marked some fields as mutable.  I think
this
> > is
> > > > fine, as long as they are only used to cache the result of
> > > > onGetScaledDimensions() (in case it should match the request to
> > > onGetPixels()).
> > > > 
> > > > On the other hand, I would not say you *must* cache it.  I am fine with
> > doing
> > > > the scaling calculation twice.
> > > 
> > > We prefer the caller use the size from onGetScaledDimensions(), because
this
> > > makes the process much faster
> > 
> > Right, but this is a virtual function, and other SkCodecs expect to be able
to
> > check a couple of times, and potentially use a different scale. FWIW,
> > SkImageGenerator built on the SkCodec API, and has the method
> > computeScaledDimensions (introduced in crrev.com/1396323007) which returns
up
> to
> > two options for supported dimensions. It is possible we'll convert SkCodec's
> API
> > to match this one. If we cannot update the underlying libraries to do the
> check
> > more efficiently (is that not possible?), I suppose we'll just have to
return
> > one possible set of supported dimensions, in order to run faster and throw
> away
> > less work. Do you have a rough idea of how long it takes to call
> > getScaledDimensions? 
> > 
> > > (in such case, we only parse and render the image
> > > once). Anyhow, we add the functionality to also support different size
> > requests,
> > > but this will cause multiple re-parsing and re-rendering.
> > 
> > So this means that the fully decoded raw image is stored in the SkCodec? How
> big
> > is a typical DngImage? SkCodec was designed around the idea that it could be
> > used for a "lazy" decode - we only decode enough initially to know the size,
> and
> > then when the client wants the actual image (e.g. at draw time, or in a
> > background thread) they can call getPixels. But this means that we may keep
> > around the DngImage at times when we do not need it.
> > 
> > > 
> > > Some more info:
> > > The only reason to render the image already in onGetScaledDimensions() is:
> we
> > > can only know the valid scaled image size after rendering. (due to the way
> the
> > > preferred size is handled by DNG SDK.)
> > 
> > Did your team also write the DNG SDK? Is it possible to change that
> restriction?
> > 
> > I think with the current implementation, we should also put a note on
> > getScaledDimensions, that it is an expensive call (which holds on to a lot
of
> > RAM) for these images.
> 
> We refactor the whole logic of scaling DNG images. We now do cheap computation
> for onGetScaledDimensions() and made the onDimensionsSupported() more
reliable.
> The heavy work is now done only in onGEtPixels(). This change also implies the
> rendering of DNG image is done "lazy", only in onGetPixels().
> 
> The DNG SDK is from Adobe. We think our new solution for handling scale
request
> should be okay.
> 
> Will the onGetPixels() be called multiple time with the same resolution for
our
> use case?

In the current common use case, Android will only call onGetPixels once and then
throw away the codec.

> If not, we can even clean up all the DNG stuff (free up memory) except
> the rawStream and recreate everything from the rawStream if onGetPixels() get
> called again.

I think that makes the most sense. The only reason to call onGetPixels again
later is if we threw away the output in order to save memory. Hanging on to the
fDngImage seems to defeat that purpose.

https://codereview.chromium.org/1520403003/diff/170001/DEPS
File DEPS (right):

https://codereview.chromium.org/1520403003/diff/170001/DEPS#newcode26
DEPS:26: "third_party/externals/piex"     :
"https://skia.googlesource.com/third_party/piex/",
On 2016/01/13 15:29:41, yujieqin wrote:
> On 2016/01/12 21:54:39, scroggo wrote:
> > Why does this use a mirror? Isn't this also checked into Android? Can we use
> the
> > http://android.googlesource.com version directly?
> 
> We are in the process of getting the repository in AOSP for PIEX. Please see
> b/26146381 (just CC-ed you). It seems like we got an internal repository,
> jaesung@ is trying to find out if we can get a external ASOP/PIEX repository
> (similar to DNG SDK).
> 
> Just a quick question, would it be an issue for current CL if the AOSP/PIEX
> repository is internal?

It is only a problem if we do not have a mirror that we can use. From
b/26459537, it looks like we still do not :(

FWIW, if we don't expect ongoing development in piex, maybe we could do a manual
push, and then switch to the android.googlesource.com version once it exists -
it looks like that's been moved to b/26535406 and fixed?

If we have to stick with the one in skia.googlesource.com for now, maybe add a
FIXME that says we'll switch to AOSP when we can?

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:84: if (!safe_add_to_size_t(offset, size, &sum) ||
!this->bufferMoreData(sum)) {
Again, I don't think you need to do any buffering. Instead, why not do the
following:

const size_t alreadyBuffered = fStreamBuffer.bytesWritten() - offset;
if (!fStreamBuffer.read(data->writable_data(), alreadyBuffered)) {
    return nullptr;
}

const size_t remaining = size - alreadyBuffered;
auto* dst = static_cast<uint8_t*>(data->writable_data()) + alreadyBuffered;
if (remaining && fStream->read(dst, remaining) != remaining) {
    return nullptr;
}

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:173: static dng_negative* ReadDng(SkRawStream* stream,
dng_host* host, dng_info* info) {
I think this does not need to be public?

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:212: SkCodec::Result render(int width, int height) {
Can this just return a boolean for success or failure?

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:213: if (!fImage ||
I think saving the image across calls to onGetPixels is not necessary. (It will
run faster, but at the cost of using more memory.)

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:260: // Because the DNG SDK can not gaurantee to render
to requested size, we allow a small
nit: 100 chars

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:277: SkCodec::Result writeToBuffer(dng_pixel_buffer*
buffer) {
This only returns two values, which are essentially success or failure. Why not
return a bool?

Also, naming this "readRow" would align with some of our other codecs.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:340: SkMemoryStream* memoryStream =
This will leak in GOOGLE3. Instead, I think you want to move your #if !defined
above this line.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:371: // Only copy the top-left of the overlapping
region.
In practice, how much will be "cropped" here? If the client used values that
came from getScaledDimensions, will any cropping be necessary?

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:383: buffer.fArea = dng_rect(i, 0, i + 1, width);
Every setup assignment for buffer is the same for each iteration. I don't know
anything about dng_pixel_buffer; would it be possible to do the following?

dng_pixel_buffer buffer;
buffer.fData = &srcRow[0];
buffer.fPlane = 0;
buffer.fColoStep = buffer.fPlanes;
buffer.fPlaneStep = 1;
buffer.fPixelType = ttByte;
buffer.fPixelSize = sizeof(uint8); // (uint8_t?)
buffer.fRowStep = sizeof(srcRow);

for (int i = 0; i < height; ++i) {
    buffer.fArea = dng_rect(i, 0, i + 1, width);
}

(Or do you think the compiler will handle this for us?)

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:389: buffer.fPixelSize = sizeof(uint8);
Should this be uint8_t? (Actually, I just realized you're using "uint8" (no
"_t") everywhere. Where is this defined? Any reason to use this rather than
"uint8_t", as we do in the rest of Skia?

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:392: result = fDngImage->writeToBuffer(&buffer);
if (!fDngImage->readRow(&buffer)) {
    *rowsDecoded = y;
    return kIncompleteInput;
}

(I recommended the name/signature change at the method's declaration.)

By setting rowsDecoded and returning kIncompleteInput, we can return a partial
image which will get the rest of its rows filled by the base class.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:408: // Limits the minimun size to be 128 on the long
edge.
On 2016/01/13 17:15:14, msarett wrote:
> This limitation seems reasonable.  Is this a limit imposed by the DNG lib or
> arbitrarily by us?
> 
> My concern is that this is a different behavior than the other Codecs.
> 
> On the other hand, will RAW support all integer downscales (1, 2, 3, 5, 6
...)? 
> Because if not, we already have a difference in behavior anyway.

I think that's okay. I believe we've had other cases where we did not exactly
match the downscale.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.h...
src/codec/SkRawCodec.h:55: mutable SkAutoTDelete<SkDngImage> fDngImage;
Does fDngImage need to be mutable, now that you do not modify it
onGetScaledDimensions?

yujieqin

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp#newcode241 src/codec/SkRawCodec.cpp:241: if (!fDngImage) { On 2016/01/13 18:24:44, scroggo wrote: > ...

4 years, 11 months ago (2016-01-14 09:33:12 UTC) #40

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/13 18:24:44, scroggo wrote:
> On 2016/01/13 15:29:41, yujieqin wrote:
> > On 2016/01/12 21:54:39, scroggo wrote:
> > > On 2016/01/12 10:47:40, yujieqin wrote:
> > > > On 2016/01/11 19:12:44, msarett wrote:
> > > > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > > > I think here, you are assuming that getScaledDimensions() has
> > > already
> > > > > been
> > > > > > > > > called, and then the client is calling getPixels() with the
same
> > > size
> > > > > > > returned
> > > > > > > > > by getScaledDimensions()?
> > > > > > > > > 
> > > > > > > > > In that case, I think this will work.
> > > > > > > > > 
> > > > > > > > > However, the client is not required to use the size returned
by
> > > > > > > > > getScaledDimensions().  I think this will break if the client
> > calls
> > > > > > > > > getScaledDimensions(), but then decides to call getPixels() on
a
> > > > > different
> > > > > > > > size.
> > > > > > > > > 
> > > > > > > > > I'm in favor of not repeating the
render_dng_for_prefered_size()
> > > call,
> > > > > but
> > > > > > > to
> > > > > > > > do
> > > > > > > > > that, I think you'll need to make sure the current size of
> > fDngImage
> > > > > > matches
> > > > > > > > the
> > > > > > > > > request.
> > > > > > > > 
> > > > > > > > Done.
> > > > > > > 
> > > > > > > How was this fixed?
> > > > > > 
> > > > > > It was fixed in the way that the size of the image rendered by the
> > > > > > render_dng_for_prefered_size will be checked against the requested
> size.
> > > If
> > > > > the
> > > > > > sizes are different the error kInvalidScale will be returned.
> > > > > > 
> > > > > > * So if the caller called getScaledDimensions() before and then
calls
> > > > > > onGetPixels() with exactly these size, it will work.
> > > > > > * If the caller called onGetPixels() directly, by requesting random
> > sizes
> > > > (so
> > > > > > not calling getScaledDimensions() before, or not calling 
> onGetPixels()
> > > with
> > > > > the
> > > > > > 100% size), then the fDngImage will be created on the fly by trying
to
> > > match
> > > > > the
> > > > > > requested size. If the requested size does not match the rendered
> size,
> > > then
> > > > > an
> > > > > > error will be returned.
> > > > > 
> > > > > Ah I see.  This is not quite enough though.
> > > > > 
> > > > > After the client calls getScaledDimensions(), they should still be
able
> to
> > > > call
> > > > > getPixels() on *any* valid size and get the correct result.
> > > > > 
> > > > > For example, maybe they don't like the recommended scaled dimensions,
> and
> > > > decide
> > > > > to just decode to the original size.  Or maybe they will call
> > > > > getScaledDimensions() more than once, and then choose whichever size
> they
> > > like
> > > > > best.
> > > > > 
> > > > > Essentially, onGetScaledDimensions() should behave as if it is truly
> > const. 
> > > I
> > > > > am noticing now that you have marked some fields as mutable.  I think
> this
> > > is
> > > > > fine, as long as they are only used to cache the result of
> > > > > onGetScaledDimensions() (in case it should match the request to
> > > > onGetPixels()).
> > > > > 
> > > > > On the other hand, I would not say you *must* cache it.  I am fine
with
> > > doing
> > > > > the scaling calculation twice.
> > > > 
> > > > We prefer the caller use the size from onGetScaledDimensions(), because
> this
> > > > makes the process much faster
> > > 
> > > Right, but this is a virtual function, and other SkCodecs expect to be
able
> to
> > > check a couple of times, and potentially use a different scale. FWIW,
> > > SkImageGenerator built on the SkCodec API, and has the method
> > > computeScaledDimensions (introduced in crrev.com/1396323007) which returns
> up
> > to
> > > two options for supported dimensions. It is possible we'll convert
SkCodec's
> > API
> > > to match this one. If we cannot update the underlying libraries to do the
> > check
> > > more efficiently (is that not possible?), I suppose we'll just have to
> return
> > > one possible set of supported dimensions, in order to run faster and throw
> > away
> > > less work. Do you have a rough idea of how long it takes to call
> > > getScaledDimensions? 
> > > 
> > > > (in such case, we only parse and render the image
> > > > once). Anyhow, we add the functionality to also support different size
> > > requests,
> > > > but this will cause multiple re-parsing and re-rendering.
> > > 
> > > So this means that the fully decoded raw image is stored in the SkCodec?
How
> > big
> > > is a typical DngImage? SkCodec was designed around the idea that it could
be
> > > used for a "lazy" decode - we only decode enough initially to know the
size,
> > and
> > > then when the client wants the actual image (e.g. at draw time, or in a
> > > background thread) they can call getPixels. But this means that we may
keep
> > > around the DngImage at times when we do not need it.
> > > 
> > > > 
> > > > Some more info:
> > > > The only reason to render the image already in onGetScaledDimensions()
is:
> > we
> > > > can only know the valid scaled image size after rendering. (due to the
way
> > the
> > > > preferred size is handled by DNG SDK.)
> > > 
> > > Did your team also write the DNG SDK? Is it possible to change that
> > restriction?
> > > 
> > > I think with the current implementation, we should also put a note on
> > > getScaledDimensions, that it is an expensive call (which holds on to a lot
> of
> > > RAM) for these images.
> > 
> > We refactor the whole logic of scaling DNG images. We now do cheap
computation
> > for onGetScaledDimensions() and made the onDimensionsSupported() more
> reliable.
> > The heavy work is now done only in onGEtPixels(). This change also implies
the
> > rendering of DNG image is done "lazy", only in onGetPixels().
> > 
> > The DNG SDK is from Adobe. We think our new solution for handling scale
> request
> > should be okay.
> > 
> > Will the onGetPixels() be called multiple time with the same resolution for
> our
> > use case?
> 
> In the current common use case, Android will only call onGetPixels once and
then
> throw away the codec.
> 
> > If not, we can even clean up all the DNG stuff (free up memory) except
> > the rawStream and recreate everything from the rawStream if onGetPixels()
get
> > called again.
> 
> I think that makes the most sense. The only reason to call onGetPixels again
> later is if we threw away the output in order to save memory. Hanging on to
the
> fDngImage seems to defeat that purpose.

Optimized the workflow for the that onGetPixels() will be called only once.
(But one can still call onGetPixels() multiple time, but it is not efficient.)

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:84: if (!safe_add_to_size_t(offset, size, &sum) ||
!this->bufferMoreData(sum)) {
On 2016/01/13 18:24:45, scroggo wrote:
> Again, I don't think you need to do any buffering. Instead, why not do the
> following:
> 
> const size_t alreadyBuffered = fStreamBuffer.bytesWritten() - offset;
> if (!fStreamBuffer.read(data->writable_data(), alreadyBuffered)) {
>     return nullptr;
> }
> 
> const size_t remaining = size - alreadyBuffered;
> auto* dst = static_cast<uint8_t*>(data->writable_data()) + alreadyBuffered;
> if (remaining && fStream->read(dst, remaining) != remaining) {
>     return nullptr;
> }

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:173: static dng_negative* ReadDng(SkRawStream* stream,
dng_host* host, dng_info* info) {
On 2016/01/13 18:24:45, scroggo wrote:
> I think this does not need to be public?

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:212: SkCodec::Result render(int width, int height) {
On 2016/01/13 18:24:45, scroggo wrote:
> Can this just return a boolean for success or failure?

Changed this method to return pointer.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:213: if (!fImage ||
On 2016/01/13 18:24:45, scroggo wrote:
> I think saving the image across calls to onGetPixels is not necessary. (It
will
> run faster, but at the cost of using more memory.)

Changed the logic to return dng_image directly and clean up host, info,
negative.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:260: // Because the DNG SDK can not gaurantee to render
to requested size, we allow a small
On 2016/01/13 18:24:45, scroggo wrote:
> nit: 100 chars

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:277: SkCodec::Result writeToBuffer(dng_pixel_buffer*
buffer) {
On 2016/01/13 18:24:45, scroggo wrote:
> This only returns two values, which are essentially success or failure. Why
not
> return a bool?
> 
> Also, naming this "readRow" would align with some of our other codecs.

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:340: SkMemoryStream* memoryStream =
On 2016/01/13 18:24:45, scroggo wrote:
> This will leak in GOOGLE3. Instead, I think you want to move your #if !defined
> above this line.

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:371: // Only copy the top-left of the overlapping
region.
On 2016/01/13 18:24:45, scroggo wrote:
> In practice, how much will be "cropped" here? If the client used values that
> came from getScaledDimensions, will any cropping be necessary?

Our experiment shows that the cropping will be less than 2%, even the client
used the value from getScaledDimensions(). See comment in onGetPiexels() and the
check for the 2%.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:383: buffer.fArea = dng_rect(i, 0, i + 1, width);
On 2016/01/13 18:24:45, scroggo wrote:
> Every setup assignment for buffer is the same for each iteration. I don't know
> anything about dng_pixel_buffer; would it be possible to do the following?
> 
> dng_pixel_buffer buffer;
> buffer.fData = &srcRow[0];
> buffer.fPlane = 0;
> buffer.fColoStep = buffer.fPlanes;
> buffer.fPlaneStep = 1;
> buffer.fPixelType = ttByte;
> buffer.fPixelSize = sizeof(uint8); // (uint8_t?)
> buffer.fRowStep = sizeof(srcRow);
> 
> for (int i = 0; i < height; ++i) {
>     buffer.fArea = dng_rect(i, 0, i + 1, width);
> }
> 
> (Or do you think the compiler will handle this for us?)

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:389: buffer.fPixelSize = sizeof(uint8);
On 2016/01/13 18:24:45, scroggo wrote:
> Should this be uint8_t? (Actually, I just realized you're using "uint8" (no
> "_t") everywhere. Where is this defined? Any reason to use this rather than
> "uint8_t", as we do in the rest of Skia?

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:392: result = fDngImage->writeToBuffer(&buffer);
On 2016/01/13 18:24:45, scroggo wrote:
> if (!fDngImage->readRow(&buffer)) {
>     *rowsDecoded = y;
>     return kIncompleteInput;
> }
> 
> (I recommended the name/signature change at the method's declaration.)
> 
> By setting rowsDecoded and returning kIncompleteInput, we can return a partial
> image which will get the rest of its rows filled by the base class.

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:408: // Limits the minimun size to be 128 on the long
edge.
On 2016/01/13 17:15:14, msarett wrote:
> This limitation seems reasonable.  Is this a limit imposed by the DNG lib or
> arbitrarily by us?
> 
> My concern is that this is a different behavior than the other Codecs.
> 
> On the other hand, will RAW support all integer downscales (1, 2, 3, 5, 6
...)? 
> Because if not, we already have a difference in behavior anyway.

Our experiment shows that the DNG SDK does not scale properly when the size is
smaller than 128.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:422: return dim.width() >= 1 && dim.width() <=
info.width()
On 2016/01/13 17:15:14, msarett wrote:
> Everything below this line is dead code?

Done.

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.h
File src/codec/SkRawCodec.h (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.h...
src/codec/SkRawCodec.h:55: mutable SkAutoTDelete<SkDngImage> fDngImage;
On 2016/01/13 18:24:45, scroggo wrote:
> Does fDngImage need to be mutable, now that you do not modify it
> onGetScaledDimensions?

Done.

yujieqin

https://codereview.chromium.org/1520403003/diff/170001/DEPS File DEPS (right): https://codereview.chromium.org/1520403003/diff/170001/DEPS#newcode26 DEPS:26: "third_party/externals/piex" : "https://skia.googlesource.com/third_party/piex/", On 2016/01/13 18:24:45, scroggo wrote: > ...

4 years, 11 months ago (2016-01-14 10:21:50 UTC) #41

yujieqin

yujieqin@google.com changed reviewers: - ebrauer@google.com

4 years, 11 months ago (2016-01-14 12:25:23 UTC) #42

yujieqin

Hi Leon, hi Matt, as we already got both DNG SDK and PIEX into the ...

4 years, 11 months ago (2016-01-14 12:25:24 UTC) #43

scroggo

I see you added an image to resources/. Please add a test to test/CodexTest.cpp. We ...

4 years, 11 months ago (2016-01-14 15:26:38 UTC) #44

I see you added an image to resources/. Please add a test to test/CodexTest.cpp.
We already have a bunch of tests that check various images, so a lot of the work
is done for you.

In the section that starts "DEF_TEST(Codec, r)", you'll need to add a line that
says something like:

// RAW
check(r, "resources/sample_1mp.dng", SkISize::Make(w, h), false, false)

And a line in the section that starts "DEF_TEST(Codec_Dimensions, r)" that says:

// RAW
test_dimensions(r, "resources/sample_1mp.dng");

It might be good to test more than one file; I'm not sure what the right number
is. It would be ideal to make sure your various code paths are covered. It
sounds like this covers a lot of different file types, and I don't want to fill
up resources/ with a ton of images.

Images don't go into resources/ should go into Google Storage. (These will be
tested by DM on our infrastructure, and we can view the results in gold.skia.org
to verify that they look as expected.) We can help with that, but we need to
know which images to put there. This should be a bigger set, since it doesn't
need to be checked out with the repository. Is
https://drive.google.com/corp/drive/u/0/folders/0B-jIzwWRo27VV2ZjYWlmaE83dVk
(Skia_RAW_image_set) a good set to use?

On Thu, Jan 14, 2016 at 7:25 AM, <yujieqin@google.com> wrote:
> as we already got both DNG SDK and PIEX into the external AOSP repository, I
> think current CL don't wait for other works. Can we finalize it and get it
> submitted soon?

This is a priority for me as well, but I want to make sure this is ready. I
think it's close!

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/90001/src/codec/SkRawCodec.cp...
src/codec/SkRawCodec.cpp:241: if (!fDngImage) {
On 2016/01/14 09:33:11, yujieqin wrote:
> On 2016/01/13 18:24:44, scroggo wrote:
> > On 2016/01/13 15:29:41, yujieqin wrote:
> > > On 2016/01/12 21:54:39, scroggo wrote:
> > > > On 2016/01/12 10:47:40, yujieqin wrote:
> > > > > On 2016/01/11 19:12:44, msarett wrote:
> > > > > > On 2016/01/11 17:57:05, adaubert wrote:
> > > > > > > On 2016/01/11 17:44:02, scroggo wrote:
> > > > > > > > On 2016/01/11 14:03:07, yujieqin wrote:
> > > > > > > > > On 2016/01/08 14:38:34, msarett wrote:
> > > > > > > > > > I think here, you are assuming that getScaledDimensions()
has
> > > > already
> > > > > > been
> > > > > > > > > > called, and then the client is calling getPixels() with the
> same
> > > > size
> > > > > > > > returned
> > > > > > > > > > by getScaledDimensions()?
> > > > > > > > > > 
> > > > > > > > > > In that case, I think this will work.
> > > > > > > > > > 
> > > > > > > > > > However, the client is not required to use the size returned
> by
> > > > > > > > > > getScaledDimensions().  I think this will break if the
client
> > > calls
> > > > > > > > > > getScaledDimensions(), but then decides to call getPixels()
on
> a
> > > > > > different
> > > > > > > > > size.
> > > > > > > > > > 
> > > > > > > > > > I'm in favor of not repeating the
> render_dng_for_prefered_size()
> > > > call,
> > > > > > but
> > > > > > > > to
> > > > > > > > > do
> > > > > > > > > > that, I think you'll need to make sure the current size of
> > > fDngImage
> > > > > > > matches
> > > > > > > > > the
> > > > > > > > > > request.
> > > > > > > > > 
> > > > > > > > > Done.
> > > > > > > > 
> > > > > > > > How was this fixed?
> > > > > > > 
> > > > > > > It was fixed in the way that the size of the image rendered by the
> > > > > > > render_dng_for_prefered_size will be checked against the requested
> > size.
> > > > If
> > > > > > the
> > > > > > > sizes are different the error kInvalidScale will be returned.
> > > > > > > 
> > > > > > > * So if the caller called getScaledDimensions() before and then
> calls
> > > > > > > onGetPixels() with exactly these size, it will work.
> > > > > > > * If the caller called onGetPixels() directly, by requesting
random
> > > sizes
> > > > > (so
> > > > > > > not calling getScaledDimensions() before, or not calling 
> > onGetPixels()
> > > > with
> > > > > > the
> > > > > > > 100% size), then the fDngImage will be created on the fly by
trying
> to
> > > > match
> > > > > > the
> > > > > > > requested size. If the requested size does not match the rendered
> > size,
> > > > then
> > > > > > an
> > > > > > > error will be returned.
> > > > > > 
> > > > > > Ah I see.  This is not quite enough though.
> > > > > > 
> > > > > > After the client calls getScaledDimensions(), they should still be
> able
> > to
> > > > > call
> > > > > > getPixels() on *any* valid size and get the correct result.
> > > > > > 
> > > > > > For example, maybe they don't like the recommended scaled
dimensions,
> > and
> > > > > decide
> > > > > > to just decode to the original size.  Or maybe they will call
> > > > > > getScaledDimensions() more than once, and then choose whichever size
> > they
> > > > like
> > > > > > best.
> > > > > > 
> > > > > > Essentially, onGetScaledDimensions() should behave as if it is truly
> > > const. 
> > > > I
> > > > > > am noticing now that you have marked some fields as mutable.  I
think
> > this
> > > > is
> > > > > > fine, as long as they are only used to cache the result of
> > > > > > onGetScaledDimensions() (in case it should match the request to
> > > > > onGetPixels()).
> > > > > > 
> > > > > > On the other hand, I would not say you *must* cache it.  I am fine
> with
> > > > doing
> > > > > > the scaling calculation twice.
> > > > > 
> > > > > We prefer the caller use the size from onGetScaledDimensions(),
because
> > this
> > > > > makes the process much faster
> > > > 
> > > > Right, but this is a virtual function, and other SkCodecs expect to be
> able
> > to
> > > > check a couple of times, and potentially use a different scale. FWIW,
> > > > SkImageGenerator built on the SkCodec API, and has the method
> > > > computeScaledDimensions (introduced in crrev.com/1396323007) which
returns
> > up
> > > to
> > > > two options for supported dimensions. It is possible we'll convert
> SkCodec's
> > > API
> > > > to match this one. If we cannot update the underlying libraries to do
the
> > > check
> > > > more efficiently (is that not possible?), I suppose we'll just have to
> > return
> > > > one possible set of supported dimensions, in order to run faster and
throw
> > > away
> > > > less work. Do you have a rough idea of how long it takes to call
> > > > getScaledDimensions? 
> > > > 
> > > > > (in such case, we only parse and render the image
> > > > > once). Anyhow, we add the functionality to also support different size
> > > > requests,
> > > > > but this will cause multiple re-parsing and re-rendering.
> > > > 
> > > > So this means that the fully decoded raw image is stored in the SkCodec?
> How
> > > big
> > > > is a typical DngImage? SkCodec was designed around the idea that it
could
> be
> > > > used for a "lazy" decode - we only decode enough initially to know the
> size,
> > > and
> > > > then when the client wants the actual image (e.g. at draw time, or in a
> > > > background thread) they can call getPixels. But this means that we may
> keep
> > > > around the DngImage at times when we do not need it.
> > > > 
> > > > > 
> > > > > Some more info:
> > > > > The only reason to render the image already in onGetScaledDimensions()
> is:
> > > we
> > > > > can only know the valid scaled image size after rendering. (due to the
> way
> > > the
> > > > > preferred size is handled by DNG SDK.)
> > > > 
> > > > Did your team also write the DNG SDK? Is it possible to change that
> > > restriction?
> > > > 
> > > > I think with the current implementation, we should also put a note on
> > > > getScaledDimensions, that it is an expensive call (which holds on to a
lot
> > of
> > > > RAM) for these images.
> > > 
> > > We refactor the whole logic of scaling DNG images. We now do cheap
> computation
> > > for onGetScaledDimensions() and made the onDimensionsSupported() more
> > reliable.
> > > The heavy work is now done only in onGEtPixels(). This change also implies
> the
> > > rendering of DNG image is done "lazy", only in onGetPixels().
> > > 
> > > The DNG SDK is from Adobe. We think our new solution for handling scale
> > request
> > > should be okay.
> > > 
> > > Will the onGetPixels() be called multiple time with the same resolution
for
> > our
> > > use case?
> > 
> > In the current common use case, Android will only call onGetPixels once and
> then
> > throw away the codec.
> > 
> > > If not, we can even clean up all the DNG stuff (free up memory) except
> > > the rawStream and recreate everything from the rawStream if onGetPixels()
> get
> > > called again.
> > 
> > I think that makes the most sense. The only reason to call onGetPixels again
> > later is if we threw away the output in order to save memory. Hanging on to
> the
> > fDngImage seems to defeat that purpose.
> 
> Optimized the workflow for the that onGetPixels() will be called only once.
> (But one can still call onGetPixels() multiple time, but it is not efficient.)

Yeah, I think it's fine that it's not efficient. Again, the only reason to call
it again is because we wanted to free up memory. Caching the (larger) dng_image
defeats that purpose.

https://codereview.chromium.org/1520403003/diff/170001/DEPS
File DEPS (right):

https://codereview.chromium.org/1520403003/diff/170001/DEPS#newcode26
DEPS:26: "third_party/externals/piex"     :
"https://skia.googlesource.com/third_party/piex/",
On 2016/01/14 10:21:50, yujieqin wrote:
> On 2016/01/13 18:24:45, scroggo wrote:
> > On 2016/01/13 15:29:41, yujieqin wrote:
> > > On 2016/01/12 21:54:39, scroggo wrote:
> > > > Why does this use a mirror? Isn't this also checked into Android? Can we
> use
> > > the
> > > > http://android.googlesource.com version directly?
> > > 
> > > We are in the process of getting the repository in AOSP for PIEX. Please
see
> > > b/26146381 (just CC-ed you). It seems like we got an internal repository,
> > > jaesung@ is trying to find out if we can get a external ASOP/PIEX
repository
> > > (similar to DNG SDK).
> > > 
> > > Just a quick question, would it be an issue for current CL if the
AOSP/PIEX
> > > repository is internal?
> > 
> > It is only a problem if we do not have a mirror that we can use. From
> > b/26459537, it looks like we still do not :(
> > 
> > FWIW, if we don't expect ongoing development in piex, maybe we could do a
> manual
> > push, and then switch to the http://android.googlesource.com version once it
> exists -
> > it looks like that's been moved to b/26535406 and fixed?
> > 
> > If we have to stick with the one in http://skia.googlesource.com for now,
> maybe add a
> > FIXME that says we'll switch to AOSP when we can?
> 
> We finally got the external/piex in AOSP. :) Updated here.

Great. I closed b/26459537

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/190001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:408: // Limits the minimun size to be 128 on the long
edge.
On 2016/01/14 09:33:11, yujieqin wrote:
> On 2016/01/13 17:15:14, msarett wrote:
> > This limitation seems reasonable.  Is this a limit imposed by the DNG lib or
> > arbitrarily by us?
> > 
> > My concern is that this is a different behavior than the other Codecs.
> > 
> > On the other hand, will RAW support all integer downscales (1, 2, 3, 5, 6
> ...)? 
> > Because if not, we already have a difference in behavior anyway.
> 
> Our experiment shows that the DNG SDK does not scale properly when the size is
> smaller than 128.

Can you add a comment explaining that? Also, is it worth filing a bug against
the DNG SDK? (And referencing that bug here?)

If we need to be able to get smaller images, we can scale afterwards. (I almost
suggested scanline decoding, but since we'll need to have the entire dng_image
for the duration of the scanline decode, we might as well use the drawing code.
The drawbacks to the drawing code are that we cannot have an output of index8 or
unpremul, but we won't return index8 here anyway, and these images are opaque,
so they can be treated as unpremul.)

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp#newcode74
gyp/codec.gyp:74: 'target_name': 'raw_codec',
Please add a comment here and elsewhere (e.g. where skia_codec_decodes_raw is
defined) that raw_codec is its own target because it requires exceptions,
whereas the rest of the code does not.

(As a side note, our current tool for generating our Android makefile pools all
cflags together, so this means that *all* files will be built with -fexceptions.
I'm not sure what the implications of that are.)

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp#newcode106
gyp/codec.gyp:106: 'TURBO_HAS_SKIP',
Is this necessary here? It does not directly check this compile time flag, does
it?

https://codereview.chromium.org/1520403003/diff/250001/gyp/dng_sdk.gyp
File gyp/dng_sdk.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/dng_sdk.gyp#newcode8
gyp/dng_sdk.gyp:8: 'skia_warnings_as_errors': 0,
I think this should go inside dng_sdk, rather than here.

https://codereview.chromium.org/1520403003/diff/250001/gyp/piex.gyp
File gyp/piex.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/piex.gyp#newcode16
gyp/piex.gyp:16: 'skia_warnings_as_errors': 0,
Again, I don't think this is necessary here. Instead, I think it should go
inside the piex target.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkAndroidCod...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:23: : fInfo(codec->getInfo()), fCodec(codec) {}
This change is unnecessary.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:94: if (!codec && streamDeleter) {
As stated before, we should never have a codec if (streamDeleter). This code
should instead say:

if (streamDeleter) {
    SkASSERT(!codec);

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:64: * Creates a SkMemoryStream from the offset with
size.
nit: an SkMemoryStream

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:68: SkMemoryStream* copyBuffer(size_t offset, size_t
size) {
Maybe this should have a more descriptive name that copyBuffer. It no longer
necessarily copies a buffer, but it does create an SkMemoryStream and destroy
the SkRawStream. That said, I do not have an idea for a better name.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:78: return nullptr;
I suggested this, and it matches the intent of the code you originally wrote.
But it occurred to me that we generally will decode incomplete images. If we
want to display the partial JPEG (we won't know how complete it is until
SkJpegCodec gets a hold of it) we can do the following:

const size_t bytesRead = fStream->read(data->writable_data(), size));
if (bytesRead < size) {
    data.reset(SkData::NewSubset(data.get(), 0, bytesRead));
}

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:88: if (remaining && fStream->read(dst, remaining) !=
remaining) {
The above comment ("This is destructive to the current object") is true here as
well.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:89: return nullptr;
Similar to the above case, we can pass what is available to SkJpegCodec if we
read less than "remaining" bytes.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:186: * The rendered image will be close to the
specified size, but there is no guarantee that any
So long as the specified size is greater than 128 on the long edge (DNG SDK bug
###)

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:193: // render() takes the ownership of fHost, fInfo
and fNegative when available.
nit: takes ownership* (no need for "the" here)

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:245: SkImageInfo getImageInfo() const {
nit: You can make this return const SkImageInfo& to avoid a copy.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const
SkImageInfo& requestedInfo, void* dst,
This function needs to check to see that the requestedInfo's colortype is okay.
For other codecs, we do this in conversion_possible in SkCodecPriv.h

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:332: // Because the DNG SDK can not gaurantee to render
to requested size, we allow a small
guarantee*

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:333: // difference. Only the overlapping region will be
copied in onGetPixels().
nit: It's not really a copy - it looks like it's a two step conversion:
- from the format of dng_image to srcRow
- from srcRow to the output memory

Which reminds me, would it be possible to skip one of these steps? i.e. can we
convert directly from the dng_image to the output memory, either using options
of dng_image or by adding modes to the swizzler?

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:341: // Only copy the top-left of the overlapping
region.
Again, I think "copy" is misleading here.

msarett

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp#newcode336 src/codec/SkRawCodec.cpp:336: if (SkTMax(width, imageSize.h) / SkTMin(width, imageSize.h) > maxDiffRatio || ...

4 years, 11 months ago (2016-01-14 19:02:14 UTC) #45

yujieqin

* Add the two tests as you mentioned. It builds, but I was not able ...

4 years, 11 months ago (2016-01-15 14:49:31 UTC) #46

* Add the two tests as you mentioned. It builds, but I was not able to verify if
it runs properly.
* The mentioned Drive folder is correct. It now contains all the interesting RAW
images which cover all relevant code paths.

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp#newcode74
gyp/codec.gyp:74: 'target_name': 'raw_codec',
On 2016/01/14 15:26:38, scroggo wrote:
> Please add a comment here and elsewhere (e.g. where skia_codec_decodes_raw is
> defined) that raw_codec is its own target because it requires exceptions,
> whereas the rest of the code does not.
> 
> (As a side note, our current tool for generating our Android makefile pools
all
> cflags together, so this means that *all* files will be built with
-fexceptions.
> I'm not sure what the implications of that are.)

Comments added.

https://codereview.chromium.org/1520403003/diff/250001/gyp/codec.gyp#newcode106
gyp/codec.gyp:106: 'TURBO_HAS_SKIP',
On 2016/01/14 15:26:38, scroggo wrote:
> Is this necessary here? It does not directly check this compile time flag,
does
> it?

Removed.

https://codereview.chromium.org/1520403003/diff/250001/gyp/dng_sdk.gyp
File gyp/dng_sdk.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/dng_sdk.gyp#newcode8
gyp/dng_sdk.gyp:8: 'skia_warnings_as_errors': 0,
On 2016/01/14 15:26:38, scroggo wrote:
> I think this should go inside dng_sdk, rather than here.

Done.

https://codereview.chromium.org/1520403003/diff/250001/gyp/piex.gyp
File gyp/piex.gyp (right):

https://codereview.chromium.org/1520403003/diff/250001/gyp/piex.gyp#newcode16
gyp/piex.gyp:16: 'skia_warnings_as_errors': 0,
On 2016/01/14 15:26:38, scroggo wrote:
> Again, I don't think this is necessary here. Instead, I think it should go
> inside the piex target.

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkAndroidCod...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:23: : fInfo(codec->getInfo()), fCodec(codec) {}
On 2016/01/14 15:26:38, scroggo wrote:
> This change is unnecessary.

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:94: if (!codec && streamDeleter) {
On 2016/01/14 15:26:38, scroggo wrote:
> As stated before, we should never have a codec if (streamDeleter). This code
> should instead say:
> 
> if (streamDeleter) {
>     SkASSERT(!codec);

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:64: * Creates a SkMemoryStream from the offset with
size.
On 2016/01/14 15:26:38, scroggo wrote:
> nit: an SkMemoryStream

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:68: SkMemoryStream* copyBuffer(size_t offset, size_t
size) {
On 2016/01/14 15:26:38, scroggo wrote:
> Maybe this should have a more descriptive name that copyBuffer. It no longer
> necessarily copies a buffer, but it does create an SkMemoryStream and destroy
> the SkRawStream. That said, I do not have an idea for a better name.

Let's call it transferBuffer().

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:78: return nullptr;
On 2016/01/14 15:26:38, scroggo wrote:
> I suggested this, and it matches the intent of the code you originally wrote.
> But it occurred to me that we generally will decode incomplete images. If we
> want to display the partial JPEG (we won't know how complete it is until
> SkJpegCodec gets a hold of it) we can do the following:
> 
> const size_t bytesRead = fStream->read(data->writable_data(), size));
> if (bytesRead < size) {
>     data.reset(SkData::NewSubset(data.get(), 0, bytesRead));
> }

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:88: if (remaining && fStream->read(dst, remaining) !=
remaining) {
On 2016/01/14 15:26:38, scroggo wrote:
> The above comment ("This is destructive to the current object") is true here
as
> well.

true, I just removed the line 72, and we had comment about this of the method
already.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:89: return nullptr;
On 2016/01/14 15:26:38, scroggo wrote:
> Similar to the above case, we can pass what is available to SkJpegCodec if we
> read less than "remaining" bytes.

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:186: * The rendered image will be close to the
specified size, but there is no guarantee that any
On 2016/01/14 15:26:38, scroggo wrote:
> So long as the specified size is greater than 128 on the long edge (DNG SDK
bug
> ###)

We update the comment, but we did not add a bug for DNG SDK, because we don't
think there is anything to do on the DNG SDK side.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:193: // render() takes the ownership of fHost, fInfo
and fNegative when available.
On 2016/01/14 15:26:38, scroggo wrote:
> nit: takes ownership* (no need for "the" here)

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:193: // render() takes the ownership of fHost, fInfo
and fNegative when available.
On 2016/01/14 15:26:38, scroggo wrote:
> nit: takes ownership* (no need for "the" here)

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:245: SkImageInfo getImageInfo() const {
On 2016/01/14 15:26:38, scroggo wrote:
> nit: You can make this return const SkImageInfo& to avoid a copy.

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const
SkImageInfo& requestedInfo, void* dst,
On 2016/01/14 15:26:38, scroggo wrote:
> This function needs to check to see that the requestedInfo's colortype is
okay.
> For other codecs, we do this in conversion_possible in SkCodecPriv.h

The DNG SDK returns RGB 8bit data. Currently we are using the SkSwizzler to
convert it to requested info and options. Could you be more specified with the
check you are mentioning?

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:332: // Because the DNG SDK can not gaurantee to render
to requested size, we allow a small
On 2016/01/14 15:26:38, scroggo wrote:
> guarantee*

Done.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:333: // difference. Only the overlapping region will be
copied in onGetPixels().
On 2016/01/14 15:26:38, scroggo wrote:
> nit: It's not really a copy - it looks like it's a two step conversion:
> - from the format of dng_image to srcRow
> - from srcRow to the output memory
> 
> Which reminds me, would it be possible to skip one of these steps? i.e. can we
> convert directly from the dng_image to the output memory, either using options
> of dng_image or by adding modes to the swizzler?

It seems like we can not avoid these two step conversions. Because the DNG SDK
is lack of options for the conversions. Depending on your other question about
checking the requestedInfo's colortype, we might be able to do something
different. Could you postpone this optimization after we got the first round of
this CL submitted?

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:336: if (SkTMax(width, imageSize.h) / SkTMin(width,
imageSize.h) > maxDiffRatio ||
On 2016/01/14 19:02:14, msarett wrote:
> If width is greater than imageSize.h and/or height is greater than
imageSize.v,
> will we leave dst memory uninitialized?
> 
> Even if we fill it with something, won't that look weird?

Changed logic, we don't have this case anymore.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:341: // Only copy the top-left of the overlapping
region.
On 2016/01/14 15:26:38, scroggo wrote:
> Again, I think "copy" is misleading here.

we don't need this anymore.

yujieqin

FYI, we are planning to stay longer on Monday, so that we may quickly reply ...

4 years, 11 months ago (2016-01-15 15:01:53 UTC) #47

msarett

This looks good to me minus my latest comments. I believe that Leon is out ...

4 years, 11 months ago (2016-01-15 15:15:32 UTC) #48

yujieqin

Thanks for the info. :) https://codereview.chromium.org/1520403003/diff/270001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1520403003/diff/270001/gyp/codec.gyp#newcode74 gyp/codec.gyp:74: # RAW codec needs ...

4 years, 11 months ago (2016-01-15 15:47:17 UTC) #49

scroggo

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp#newcode321 src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, On 2016/01/15 14:49:30, ...

4 years, 11 months ago (2016-01-19 21:05:45 UTC) #50

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const
SkImageInfo& requestedInfo, void* dst,
On 2016/01/15 14:49:30, yujieqin wrote:
> On 2016/01/14 15:26:38, scroggo wrote:
> > This function needs to check to see that the requestedInfo's colortype is
> okay.
> > For other codecs, we do this in conversion_possible in SkCodecPriv.h
> 
> The DNG SDK returns RGB 8bit data. Currently we are using the SkSwizzler to
> convert it to requested info and options. Could you be more specified with the
> check you are mentioning? 

The check is to early exit. Yes, the SkSwizzler does the conversion, but you
assert that you have created an SkSwizzler. If the caller requested kIndex8, the
SkSwizzler will be NULL. We could rely on that happening (but we'd need to
change the assert to a branch). Elsewhere we call conversion_supported; if it
returns false, we never attempt to create the swizzler. Look at that method in
SkCodecPriv.h (it's short and straightforward) and where it's called (in the
beginning of onGetPixels in other codec implementations). The beginning of this
method should have the same call.

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:333: // difference. Only the overlapping region will be
copied in onGetPixels().
On 2016/01/15 14:49:30, yujieqin wrote:
> On 2016/01/14 15:26:38, scroggo wrote:
> > nit: It's not really a copy - it looks like it's a two step conversion:
> > - from the format of dng_image to srcRow
> > - from srcRow to the output memory
> > 
> > Which reminds me, would it be possible to skip one of these steps? i.e. can
we
> > convert directly from the dng_image to the output memory, either using
options
> > of dng_image or by adding modes to the swizzler?
> 
> It seems like we can not avoid these two step conversions. Because the DNG SDK
> is lack of options for the conversions. Depending on your other question about
> checking the requestedInfo's colortype, we might be able to do something
> different. Could you postpone this optimization after we got the first round
of
> this CL submitted?

It's fine if we cannot avoid the two step conversion. I just speculated that it
might be possible, not knowing anything about DNG SDK. I had two ideas, which
aren't directly related to my comment about requestedInfo's colorType:

1. Convert directly to 32 bit (or 565) in DNG SDK
Again, I do not know enough about DNG SDK to know whether this is possible.
(libjpeg-turbo lets us convert directly to 565, and I think 32 bit, meaning we
do not need an SkSwizzler if we are not sampling.) Even if it is possible, that
does not mean we should do it. (For example, libpng can do this, but it's slower
than what we would do otherwise.)

2. Convert directly from the image's native format
Right now, you convert from whatever the image is (I think you said it might be
16 bits per component, for example?) into RGB (with 8 bits per component), which
the SkSwizzler converts into what we want. But we could also theoretically add a
new type to SkSwizzler::SrcConfig which specifies another input format (like 16
bits per component). Then we could convert directly from the source type in the
SkSwizzler (for which Matt is writing SIMD optimizations). Does that make sense?

This does not need to prevent this CL from landing, but I could imagine that it
might be beneficial.

https://codereview.chromium.org/1520403003/diff/370001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/1520403003/diff/370001/gyp/common_variables.g...
gyp/common_variables.gypi:41: # RAW codec need exceptions. Due to that, it is a
separate target. Its usage can be controlled
nit: needs*

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:60: if (size > 300 * 1024 * 1024) {  // 300 MB
How did you choose this amount? Should it be configurable based on the device?

Do you expect that this will result in failing to decode valid images? Or was
the large request due to the fact that the image file was invalid?

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:102: auto* dst =
static_cast<uint8_t*>(data->writable_data()) + alreadyBuffered;
nit: this could go inside the if (remaining) block

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:107: if (safe_add_to_size_t(alreadyBuffered, bytesRead,
&newSize)) {
I think this should say if (!safe_add_to_size_t(...))?

FWIW, if this failed, it seems like we would have had a problem on the other
code paths in this method.

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:197: SkAutoTDelete<SkDngMemoryAllocator> allocator(new
SkDngMemoryAllocator);
Is it possible to avoid creating and passing around these objects? It seems
awkward that we "new" all these objects, pass them to a static function, then
hand off their ownership to a new object of the class. It also means that we
have several smaller allocations when we could instead have a single allocation.

Can SkDngImage look more like the following?:

class SkDngImage {
public:
  SkDngImage* NewFromStream(SkRawStream* stream) {
    SkAutoTDelete<SkDngImage> dngImage(new SkDngImage(stream));
    if (!dngImage->readDng()) {
      return nullptr;
    }

    SkASSERT(dngImage->fNegative);
    return dngImage.release();
  }

  dng_image* render(...) {
    // looks mostly the same, but uses member variables directly
    // is it safe to just reuse fHost and fInfo rather than creating new ones?
    // I am not familiar with the DNG SDK, so maybe it's not?
  }
private:
  bool readDng() {
    // looks mostly the same, but uses member variables instead of parameters
    // returns true if there is a dng_negative
  }

  SkDngImage(SkRawStream* stream)
    : fStream(stream)
    , fHost(&fAllocator) {}

  SkAutoTDelete<SkRawStream> fStream;
  SkDngMemoryAllocator fAllocator;
  dng_host fHost;
  dng_info fInfo;
  SkAutoTDelete<dng_negative> fNegative;
};

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:308: , fIsXtransImage(negative &&
negative->GetMosaicInfo() != nullptr
I don't think you need to check "negative" here - if it is NULL, we'll crash in
the creation of fImageInfo.

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:413: const int shortEdge = SkTMin(dim.fWidth,
dim.fHeight);
nit: you can cast to float here, simplifying the if statement below.

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:415: // Limits the minimun size to be 80 on the short
edge.
minimum*

scroggo

Are you running dm --src image --images Skia_RAW_image_set ? This will be a good way ...

4 years, 11 months ago (2016-01-19 21:13:20 UTC) #51

yujieqin

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp#newcode321 src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, I see your ...

4 years, 11 months ago (2016-01-20 12:53:04 UTC) #52

yujieqin

Oh, actually it was a bug in transferBuffer(). I fixed it in Patch Set 22. ...

4 years, 11 months ago (2016-01-20 12:56:18 UTC) #53

scroggo

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp#newcode321 src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, On 2016/01/20 12:53:03, ...

4 years, 11 months ago (2016-01-20 18:13:36 UTC) #54

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const
SkImageInfo& requestedInfo, void* dst,
On 2016/01/20 12:53:03, yujieqin wrote:
> I see your point. Added the conversion_possible() check and the check to
> (!swizzler).

With conversion_possible exiting early, you may not need the check for the
swizzler. (I don't remember for sure; there was a point when we needed to check,
before we had implemented many of them/before conversion_supported existed, but
we still do check in some other codecs. Matt, do you remember?)

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:107: if (safe_add_to_size_t(alreadyBuffered, bytesRead,
&newSize)) {
On 2016/01/20 12:53:04, yujieqin wrote:
> On 2016/01/19 21:05:44, scroggo wrote:
> > I think this should say if (!safe_add_to_size_t(...))?
> > 
> > FWIW, if this failed, it seems like we would have had a problem on the other
> > code paths in this method.
> 
> Oh yes, typo :(
> 
> But this one could potentially fail, because we got the "offset" and "size"
info
> (two numbers) directly from the image metadata. E.g. if it is a truncated
image,
> it can offer a reasonable "offset" but a huge "size".

Agreed. But won't a huge "size" cause problems in the other cases? Like on line
91:

  const size_t bytesRead = fStream->read(data->writable_data(), size);

or I guess we're depending on the stream implementation to handle that case for
us? I don't know that we've ever hardened streams against something like this.
(Although kjlubick@ is adding a fuzzer for SkCodec, which should help in this
case.)

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkAndroidCod...
File src/codec/SkAndroidCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkAndroidCod...
src/codec/SkAndroidCodec.cpp:192: 
Why did you add this extra line?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawAdapter...
File src/codec/SkRawAdapterCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawAdapter...
src/codec/SkRawAdapterCodec.cpp:2: * Copyright 2015 Google Inc.
nit: 2016.... I think? I don't know the rules for sure, but I'm guessing it
should say 2016 if we land it in 2016. Same for the other new files.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:55: virtual ~SkDngMemoryAllocator() {}
nit: I think we generally drop the virtual and use override:

    ~SkDngMemoryAllocator() override

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:98: if (alreadyBuffered > 0 &&
It looks like we forgot to consider the case where offset ==
fStreamBuffer.bytesWritten(), making alreadyBuffered zero. Did this cause a
crash?

If so, that seems like a bug in SkDynamicMemoryWStream. I think it's fine to
still have this workaround - maybe that class has a reason not to be robust? -
but maybe file a bug and reference it here?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:222: SkCodecPrintf("Warning: SkDngImage::render() is
called multiple times.");
I don't think this print statement is still necessary. I believe Matt originally
suggested it because this method was called by both onGetScaledDimensions and
onGetPixels. But now that it's only called by onGetPixels, this just means that
the client discarded their copy of the final output to save memory. So even
though this is going to be slow, I don't think the client should be doing
anything smarter to avoid it.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:377: // difference. Only the overlapping region will be
converted in onGetPixels().
nit: "in onGetPixels()" seems unnecessary here - this method *is* onGetPixels.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:382: return SkCodec::kInvalidScale;
Is this code ever reached? This gets called by SkCodec::getPixels, but only if
onDimensionsSupported() returns true. Ideally that would catch any problems so
we don't have to render the dng_image only to find out it's too big.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:425: // (stronger downscalings are fine). In this case,
returns the non-scaled version.
So this is saying that scaling by a half is not supported, but scaling to a
smaller size is okay?

In general, the motivation for downscaling is to save memory, so it's too bad
that we're suggesting the full size image when they wanted a downscaled version.
What if we instead suggested something a little smaller than a scale factor of
.5f?

Matt, I think you decided to have SkJpegCodec round up, but I do not remember
why. Is that correct?

FWIW, we should probably update SkCodec to return two sizes, like
SkImageGenerator (now) does. In which case we would give back both the full size
and whatever the smaller size is.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:436: bool SkRawCodec::onDimensionsSupported(const
SkISize& dim) {
All this scaling code seems complex. Should we add tests to verify it's doing
what we expect it to?

test_dimensions in CodexTest already checks that anything returned by
getScaledDimensions is supported (at least for sampleSizes of 1 to 32, for the
image you've checked in, although I don't know whether that's an XtransImage or
not). It doesn't give us any meaningful data about whether those scaled
dimensions made sense. I'm not sure how best to handle that, though. We could
input the sizes we expect after testing and verifying that the sizes make sense.
That would require entering a lot of sizes though...

scroggo

Why is this issue private? We've already checked in the libraries to AOSP, so it ...

4 years, 11 months ago (2016-01-20 19:21:38 UTC) #55

Why is this issue private? We've already checked in the libraries to AOSP, so it
seems like it's obvious that we're working on this, right?

A downside to making the issue private (unless I'm doing something wrong) is
that I cannot use trybots. I wanted to run a trybot to show you the error I'm
getting on mac. Here I've copied and pasted what I'm seeing locally:

FAILED: c++ -MMD -MF obj/src/codec/raw_codec.SkRawCodec.o.d -DSK_INTERNAL
-DSK_GAMMA_SRGB -DSK_GAMMA_APPLY_TO_A8 -DSK_ALLOW_STATIC_GLOBAL_INITIALIZERS=1
-DSK_SUPPORT_GPU=1 -DSK_FORCE_DISTANCE_FIELD_TEXT=0 -DSK_BUILD_FOR_MAC
-DSK_CODEC_DECODES_RAW -DSK_DEVELOPER=1 -I../../include/codec
-I../../include/private -I../../src/codec -I../../src/core -I../../include/c
-I../../include/config -I../../include/core -I../../include/pathops
-I../../include/utils/mac -I../../third_party/externals/dng_sdk/source
-I../../third_party/externals/libjpeg-turbo -I../../third_party/externals/piex
-O0 -gdwarf-2 -Werror -mmacosx-version-min=10.7 -arch x86_64 -mssse3 -Wall
-Wextra -Winit-self -Wpointer-arith -Wsign-compare -Wno-unused-parameter
-std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics
-fexceptions  -c ../../src/codec/SkRawCodec.cpp -o
obj/src/codec/raw_codec.SkRawCodec.o
In file included from ../../src/codec/SkRawCodec.cpp:26:
In file included from ../../third_party/externals/dng_sdk/source/dng_info.h:26:
In file included from ../../third_party/externals/dng_sdk/source/dng_ifd.h:25:
In file included from
../../third_party/externals/dng_sdk/source/dng_fingerprint.h:28:
In file included from
../../third_party/externals/dng_sdk/source/dng_stream.h:29:
../../third_party/externals/dng_sdk/source/dng_utils.h:32:16: error: unknown
attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
__attribute__((no_sanitize("unsigned-integer-overflow")))
               ^
../../third_party/externals/dng_sdk/source/dng_utils.h:490:16: error: unknown
attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
__attribute__((no_sanitize("float-cast-overflow")))
               ^
../../third_party/externals/dng_sdk/source/dng_utils.h:1142:16: error: unknown
attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
__attribute__((no_sanitize("unsigned-integer-overflow")))
               ^
In file included from ../../src/codec/SkRawCodec.cpp:26:
In file included from ../../third_party/externals/dng_sdk/source/dng_info.h:26:
In file included from ../../third_party/externals/dng_sdk/source/dng_ifd.h:25:
../../third_party/externals/dng_sdk/source/dng_fingerprint.h:230:18: error:
unknown attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
                __attribute__((no_sanitize("unsigned-integer-overflow")))
                               ^
../../third_party/externals/dng_sdk/source/dng_fingerprint.h:246:18: error:
unknown attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
                __attribute__((no_sanitize("unsigned-integer-overflow")))
                               ^
../../third_party/externals/dng_sdk/source/dng_fingerprint.h:262:18: error:
unknown attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
                __attribute__((no_sanitize("unsigned-integer-overflow")))
                               ^
../../third_party/externals/dng_sdk/source/dng_fingerprint.h:278:18: error:
unknown attribute 'no_sanitize' ignored [-Werror,-Wunknown-attributes]
                __attribute__((no_sanitize("unsigned-integer-overflow")))
                               ^
7 errors generated.

scroggo

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp#newcode94 src/codec/SkCodec.cpp:94: if (streamDeleter) { When you rebase, we have removed ...

4 years, 11 months ago (2016-01-20 19:47:16 UTC) #56

msarett

I ran some tests locally on the RAW images from Drive. This has raised some ...

4 years, 11 months ago (2016-01-20 20:23:17 UTC) #57

I ran some tests locally on the RAW images from Drive.

This has raised some memory use concerns.  I realize that I am late bringing
these up and that many of these are quite complicated to fix (involves stream
implementations, PIEX, DNG SDK).

I'm ok with detailed FIXMEs for now.  But I think that these are things that we
should be aware of.

https://codereview.chromium.org/1520403003/diff/410001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1520403003/diff/410001/gyp/codec.gyp#newcode110
gyp/codec.gyp:110: ['OS == "ios" or OS == "mac"', {
nit: skia_os

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawAdapter...
File src/codec/SkRawAdapterCodec.h (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawAdapter...
src/codec/SkRawAdapterCodec.h:33: bool onGetSupportedSubset(SkIRect*
desiredSubset) const override { return false; }
I believe that this needs to be /*desiredSubset*/ to avoid unused parameter
warnings/errors in Android.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:328: ::piex::Error error =
::piex::GetPreviewImageData(rawStream.get(), &imageData);
This has the potential to "reallocate" the SkRawStream->fStreamBuffer over and
over again.  See the sequence of calls from Canon_5D2.dng:

::piex::IsRaw
bufferMoreData 5000
::piex::GetPreviewImageData
bufferMoreData 108308
bufferMoreData 108310
bufferMoreData 108312
bufferMoreData 108316
bufferMoreData 108322
bufferMoreData 108324
bufferMoreData 108328
bufferMoreData 108334
bufferMoreData 108336
bufferMoreData 108340
bufferMoreData 108346
bufferMoreData 108348
bufferMoreData 108352
bufferMoreData 108358
bufferMoreData 108360
bufferMoreData 108364
bufferMoreData 108366
bufferMoreData 108370
bufferMoreData 108372
bufferMoreData 108376
bufferMoreData 108378
bufferMoreData 108382
bufferMoreData 108384
bufferMoreData 108388
bufferMoreData 108394
bufferMoreData 108396
bufferMoreData 108400
bufferMoreData 108406
bufferMoreData 108408
bufferMoreData 108412
bufferMoreData 108418
bufferMoreData 108420
bufferMoreData 108424
bufferMoreData 108430
bufferMoreData 108432
bufferMoreData 108436
bufferMoreData 108442
bufferMoreData 108444
bufferMoreData 108448
bufferMoreData 108454
bufferMoreData 108456
bufferMoreData 108460
bufferMoreData 108466
bufferMoreData 108468
bufferMoreData 108472
bufferMoreData 108478
bufferMoreData 108480
bufferMoreData 108484
bufferMoreData 108490
bufferMoreData 108492
bufferMoreData 108496
bufferMoreData 108502
bufferMoreData 108504
bufferMoreData 108508
bufferMoreData 108514
bufferMoreData 108516
bufferMoreData 108520
bufferMoreData 108526
bufferMoreData 108528
bufferMoreData 108532
bufferMoreData 108538
bufferMoreData 108540
bufferMoreData 108544
bufferMoreData 108550
bufferMoreData 108552
bufferMoreData 108556
bufferMoreData 108562
bufferMoreData 108564
bufferMoreData 108568
bufferMoreData 108572
bufferMoreData 111464
bufferMoreData 111498
bufferMoreData 111500
bufferMoreData 111502
bufferMoreData 111506
bufferMoreData 111512
bufferMoreData 111514
bufferMoreData 111518
bufferMoreData 111524
bufferMoreData 111526
bufferMoreData 111530
bufferMoreData 111536
bufferMoreData 111538
bufferMoreData 111542
bufferMoreData 111548
bufferMoreData 111550
bufferMoreData 111554
bufferMoreData 111556
bufferMoreData 111560
bufferMoreData 111562
bufferMoreData 111566
bufferMoreData 111568
bufferMoreData 111572
bufferMoreData 111574
bufferMoreData 111578
bufferMoreData 111582
bufferMoreData 111584
bufferMoreData 111586
bufferMoreData 111590
bufferMoreData 111596
bufferMoreData 111598
bufferMoreData 111602
bufferMoreData 111608
bufferMoreData 111610
bufferMoreData 111614
bufferMoreData 111618
bufferMoreData 111620
bufferMoreData 111622
bufferMoreData 111626
bufferMoreData 111632
bufferMoreData 111634
bufferMoreData 111638
bufferMoreData 111644
bufferMoreData 111646
bufferMoreData 111650
bufferMoreData 111656
bufferMoreData 111658
bufferMoreData 111662
bufferMoreData 111668
bufferMoreData 111670
bufferMoreData 111674
bufferMoreData 111680
bufferMoreData 111682
bufferMoreData 111686
bufferMoreData 111692
bufferMoreData 111694
bufferMoreData 111698
bufferMoreData 111704
bufferMoreData 111706
bufferMoreData 111710
bufferMoreData 111716
bufferMoreData 111718
bufferMoreData 111722
bufferMoreData 111728
bufferMoreData 111730
bufferMoreData 111734
bufferMoreData 111742
bufferMoreData 111892
bufferMoreData 111894
bufferMoreData 111896
bufferMoreData 111900
bufferMoreData 111906
bufferMoreData 111908
bufferMoreData 111912
bufferMoreData 111918
bufferMoreData 111920
bufferMoreData 111924
bufferMoreData 111930
bufferMoreData 111932
bufferMoreData 111936
bufferMoreData 111942
bufferMoreData 111944
bufferMoreData 111948
bufferMoreData 111950
bufferMoreData 111954
bufferMoreData 111956
bufferMoreData 111960
bufferMoreData 111962
bufferMoreData 111966
bufferMoreData 111968
bufferMoreData 111972
bufferMoreData 111976
bufferMoreData 111978
bufferMoreData 111980
bufferMoreData 111984
bufferMoreData 111990
bufferMoreData 111992
bufferMoreData 111996
bufferMoreData 112002
bufferMoreData 112004
bufferMoreData 112008
bufferMoreData 112012
bufferMoreData 112014
bufferMoreData 112016
bufferMoreData 112020
bufferMoreData 112026
bufferMoreData 112028
bufferMoreData 112032
bufferMoreData 112038
bufferMoreData 112040
bufferMoreData 112044
bufferMoreData 112050
bufferMoreData 112052
bufferMoreData 112056
bufferMoreData 112062
bufferMoreData 112064
bufferMoreData 112068
bufferMoreData 112074
bufferMoreData 112076
bufferMoreData 112080
bufferMoreData 112086
bufferMoreData 112088
bufferMoreData 112092
bufferMoreData 112098
bufferMoreData 112100
bufferMoreData 112104
bufferMoreData 112110
bufferMoreData 112112
bufferMoreData 112116
bufferMoreData 112122
bufferMoreData 112124
bufferMoreData 112128
bufferMoreData 112136
bufferMoreData 112286
bufferMoreData 112288
bufferMoreData 112290
bufferMoreData 112294
bufferMoreData 112298
bufferMoreData 112610
bufferMoreData 112618
bufferMoreData 112638
bufferMoreData 112698

It's not so bad because the implementation makes good use of a linked list, but
it's not great either.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:330: if (error == ::piex::Error::kOk &&
imageData.preview_length > 0) {
Bad Case Stream Memory Use for Raw Image with Piex Preview:

Nikon_P330.NRW

Original Stream: 27 MB

Assume that the Java client buffers the stream in order to call:
BitmapFactory.decodeBounds()
rewind()
BitmapFactory.decode()

Java Buffer for Stream: 27 MB

SkRawStream will buffer the original stream as it reads up to the preview:
SkRawStream Buffer: 24.5 MB (often this is less - this is a bad case example)

Then we will copy to an SkMemoryStream for a Jpeg decode:
SkMemoryStream: 2.5 MB

So we will peak at about 81 MB (immediately before the SkRawStream is deleted).

Is this acceptable?  Can we do better?  Seems like it would be useful to know
whether or not there is a preview as soon as possible from Piex.  Because, if
there is a preview, maybe we can stop/limit our buffering?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:335:
rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length);
This operation involves allocating a new 1-4 MB buffer for the Jpeg stream, and
then copying 1-4 MB from the the fStreamBuffer into the Jpeg stream.

The creation of this new stream is followed by the deletion of the old stream
(generally much larger than 1-4 MB).

In general, I guess this is a memory win.  But it often involves a large
allocation to buffer something that is potentially already a buffer?

I think this should be cleaned up, but is quite complicated by:
(1) How much of the SkRawStream is buffered?
(2) Is the original SkStream a memory stream or something less versatile?

I think this is fine for now.  But can we add a FIXME?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:345: SkAutoTDelete<SkDngImage>
dngImage(SkDngImage::NewFromStream(rawStream.release()));
It looks to me like ::piex::IsRaw() returns true for all RAW images (even the
DNGs that don't have a preview).

Can we move this code inside that "if" statement and return nullptr here?  That
way we only have to read 5000 bytes to determine success/failure.

This will prevent us from the reading an entire, arbitrarily long garbage stream
before realizing that we don't have a RAW image.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:350: return new SkRawCodec(dngImage.release());
Bad Case Memory Use for Dng Image without Preview:

HTC.dng

Original Stream: 40 MB

Assume that the Java client buffers the stream in order to call:
BitmapFactory.decodeBounds()
rewind()
BitmapFactory.decode()

Java Buffer for Stream: 40 MB

SkRawStream Buffer for Stream: 40 MB

This peaks at about 120 MB.  This is just the stream (not including the memory
for the output image).

SkRawStream is forced to immediately buffer the whole stream because GetLength()
is always called first.  Can we somehow provide a length (or length estimate)
without reading the entire steam here?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:364: if (!swizzler) {
I think that, because conversion_possible succeeded, we should
SkASSERT(swizzler).

Only (N32, Opaque) and (565, Opaque) will be approved by conversion_possible. 
And the swizzler must support (and does support) both of those conversions from
kRGB.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:425: // (stronger downscalings are fine). In this case,
returns the non-scaled version.
On 2016/01/20 18:13:36, scroggo wrote:
> So this is saying that scaling by a half is not supported, but scaling to a
> smaller size is okay?
> 
> In general, the motivation for downscaling is to save memory, so it's too bad
> that we're suggesting the full size image when they wanted a downscaled
version.
> What if we instead suggested something a little smaller than a scale factor of
> .5f?
> 
> Matt, I think you decided to have SkJpegCodec round up, but I do not remember
> why. Is that correct?
> 

I've changed that code multiple times for multiple different reasons.  Right
now, it rounds up.  (Side Note: IIRC the reason is consistency.  libjpeg-turbo
will decode a 41x41 image at a 1/4 scale to 11x11 - it always rounds up.  So if
we also round up - a request for 0.15 goes to 1/4 instead of 1/8, the decoded
output will always be >= than the request.  This is at least easier to predict.)

What we really should do is suggest two candidates - one larger and one smaller.

> FWIW, we should probably update SkCodec to return two sizes, like
> SkImageGenerator (now) does. In which case we would give back both the full
size
> and whatever the smaller size is.

Agreed.  I have no preference for what to do right now.

Side Note 2:

In past releases, BitmapFactory has been buggy/inconsistent regarding whether it
will actually respect the sampleSize that is requested.  The new implementation
will always get within one.  Ex: 25x25 with sampleSize=2 always decodes to 12x12
OR 13x13, but never 25x25 or 6x6 etc.

RAW will not maintain that behavior, but it's undocumented and I don't
anticipate anybody will notice or care.

scroggo

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp#newcode335 src/codec/SkRawCodec.cpp:335: rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length); On 2016/01/20 20:23:17, msarett wrote: > This ...

4 years, 11 months ago (2016-01-20 20:36:24 UTC) #58

msarett

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp#newcode335 src/codec/SkRawCodec.cpp:335: rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length); On 2016/01/20 20:36:24, scroggo wrote: > On ...

4 years, 11 months ago (2016-01-20 20:43:52 UTC) #59

yujieqin

* We actually don't know which type of the issue we should set "private" or ...

4 years, 11 months ago (2016-01-20 21:24:20 UTC) #60

yujieqin

https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/250001/src/codec/SkRawCodec.cpp#newcode321 src/codec/SkRawCodec.cpp:321: SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst, From my test ...

4 years, 11 months ago (2016-01-20 21:42:20 UTC) #61

yujieqin

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp#newcode94 src/codec/SkCodec.cpp:94: if (streamDeleter) { Sorry, as I am not really ...

4 years, 11 months ago (2016-01-20 21:46:45 UTC) #62

msarett

"Sorry, as I am not really using git before, I have a dummy question: how ...

4 years, 11 months ago (2016-01-20 21:54:37 UTC) #63

adaubert

Regarding compiler issues of DNG SDK and no_sanitize: a fix was submitted to AOSP in ...

4 years, 11 months ago (2016-01-21 10:43:30 UTC) #64

msarett

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp#newcode330 src/codec/SkRawCodec.cpp:330: if (error == ::piex::Error::kOk && imageData.preview_length > 0) { ...

4 years, 11 months ago (2016-01-21 15:46:16 UTC) #65

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:330: if (error == ::piex::Error::kOk &&
imageData.preview_length > 0) {
On 2016/01/20 21:42:20, yujieqin wrote:
> The position of embedded preview is unpredictable (every camera can do what it
> what). So there is not guarantee we can win from such change as you mentioned.
> As said, we may investigate later to see if we can improve some formats. But
so
> far, as we tested in the head of Android, we don't see memory issue.

Acknowledged.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:335:
rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length);
On 2016/01/20 21:42:20, yujieqin wrote:
> Added FIXME. But, as said, the location of the embedded image is quite
various,
> we may not win too much here in general cases.

Acknowledged.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:345: SkAutoTDelete<SkDngImage>
dngImage(SkDngImage::NewFromStream(rawStream.release()));
On 2016/01/20 21:42:20, yujieqin wrote:
> Similar to the type checks in SkCodec.cpp, the ::piex::IsRaw() only offers a
> quick and dirty check, which is not 100% accurate. We did know some cases the
> PIEX can not recognize DNG.
> 
> Instead, DNG SDK is dedicated for DNG format, it uses more complicated
> validation. That is why I put it after the general PIEX cases. So that we can
> avoid missing support for some DNGs.

Understood.

Would it be possible to make it a long term goal to extend Piex/DNG_SDK to be
able to recognize all relevant RAW images without reading the entire stream?  Do
some DNGs really not indicate that they are DNGs until the end of the stream?

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:350: return new SkRawCodec(dngImage.release());
On 2016/01/20 21:42:20, yujieqin wrote:
> Unfortunately, the length is required by DNG SDK, we can not avoid that.
> 

Can we make it a longer term goal to "fix" the DNG SDK to not immediately
require the length?

> One option I think we may try later is, we could check if the data stream for
> SkRawStream is rewindable. If "Yes" (the case you mentioned), then we may
avoid
> the buffering in SkRawStream. What do you think?
> 

I think that would be great!  There are certainly cases where it will be
rewindable, and we have the opportunity to save this memory.  I'm fine if this
is a follow-up.

> Luckily we don't see real memory issue during out test on the head build of
> Android with current CL.

Acknowledged.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:364: if (!swizzler) {
On 2016/01/20 21:42:19, yujieqin wrote:
> I think the 565 does not work in current test, the swizzler == nullptr. (using
> the testing DNG images)
> 
> Did we do something wrong here about the swizzler?

No, SkSwizzler had a bug where we forgot to add the case for RGB->565.  This has
been fixed.

When you are able to rebase, you will get this fix.  Let me know if you still
need help rebasing.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:425: // (stronger downscalings are fine). In this case,
returns the non-scaled version.
On 2016/01/21 10:43:30, adaubert wrote:
> On 2016/01/20 21:42:20, yujieqin wrote:
> > Just for clarification, does it mean we can keep our current solution for
now?
> > :)
> 
> Downscaling to a smaller size, in this case 1/3, would be Ok. Like Yujie said,
> it's up to you guys what you would prefer.

Leon?

yujieqin

Description was changed from ========== Prototype of RAW decoding in Skia. Note: * Need to ...

4 years, 11 months ago (2016-01-21 16:25:57 UTC) #66

scroggo

On Wed, Jan 20, 2016 at 4:24 PM, <yujieqin@google.com> wrote: > * We actually don't ...

4 years, 11 months ago (2016-01-21 16:36:10 UTC) #67

On Wed, Jan 20, 2016 at 4:24 PM, <yujieqin@google.com> wrote:
> * We actually don't know which type of the issue we should set "private" or
> "public". And this CL is created before the open sourcing is done. What is the
> general routine here for Skia? Should I now set this CL to public? Or we can
> just keep it as it is?

Me neither. It's fine to leave as is. It just means I cannot run trybots. 

> Sorry, as I am not really using git before, I have a dummy question:
> how can I "rebase"? I tried "git rebase", and did not see any changes.
> could you let me know how can I "sync" my current CL correctly with the
> head of skia?

No need to apologize - git is very "powerful", which does not imply ease of use!

"git rebase", as I understand it, requires an argument (though there are surely
many ways to use it, as is typical with git). "git rebase master" would rebase
it onto the branch "master" (at least assuming that your branch tracks master).

You can also follow Matt's steps. There is a difference, and it's an important
one, but in our workflow (details withheld...) either one is fine.

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:107: if (safe_add_to_size_t(alreadyBuffered, bytesRead,
&newSize)) {
On 2016/01/20 21:42:19, yujieqin wrote:
> I don't think there will be issue with a huge "size". As said, the
> stream->read() should handle this case correctly. (i.e. if the requested size
>
> stream size, then fail properly.) If the steam can not guarantee this point,
it
> will be a general issue for Skia.

Streams are mostly in Skia for images and typefaces. Prior to your changes,
those were always much smaller. So my guess is that we're in new territory for
SkStream dealing with large numbers.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:98: if (alreadyBuffered > 0 &&
On 2016/01/20 21:42:20, yujieqin wrote:
> Yes, it seems like the SkDynamicMemoryWStream.read() will return "false" if
the
> requested "size  == 0". There is no crash. But we still need to distinguish
this
> case and the normal read failure,  that's why I add this workaround.
> 
> As I don't have enough knowledge about Skia framework yet, I am not sure if it
> is a bug or work as expected.

I filed skbug.com/4836 to investigate.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:330: if (error == ::piex::Error::kOk &&
imageData.preview_length > 0) {
On 2016/01/20 21:42:20, yujieqin wrote:
> The position of embedded preview is unpredictable (every camera can do what it
> what). So there is not guarantee we can win from such change as you mentioned.
> As said, we may investigate later to see if we can improve some formats. But
so
> far, as we tested in the head of Android, we don't see memory issue.

We've already done something to help this problem - transferBuffer will skip to
reach preview_offset and copy directly to the output rather than buffering. But
I don't know whether GetPreviewImageData reads all the way to that offset
anyway, in which case, yes, we would be buffering all that data unnecessarily.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:345: SkAutoTDelete<SkDngImage>
dngImage(SkDngImage::NewFromStream(rawStream.release()));
On 2016/01/20 21:42:20, yujieqin wrote:
> Similar to the type checks in SkCodec.cpp, the ::piex::IsRaw() only offers a
> quick and dirty check, which is not 100% accurate. We did know some cases the
> PIEX can not recognize DNG.

FWIW, SkCodec.cpp's type checks may offer false positives (e.g. first few bytes
looked like a PNG, but the rest is garbage). It sounds like ::piex::IsRaw also
has false negatives.

> 
> Instead, DNG SDK is dedicated for DNG format, it uses more complicated
> validation. That is why I put it after the general PIEX cases. So that we can
> avoid missing support for some DNGs.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:382: return SkCodec::kInvalidScale;
On 2016/01/21 10:43:30, adaubert wrote:
> On 2016/01/20 21:42:20, yujieqin wrote:
> > adaubert@ wrote a test to verify this part could happen.
> 
> In practice we should be safe and this case should never happen. But due to
the
> flexibility and diversity of the DNG standard, we can't guarantee that this
case
> would not happen at all.
> To ensure that we don't run into a strange case, where the rendered dng_image
is
> way larger than the requested size, we check here.

I guess that's fine. My real concern would be if getScaledDimensions returned a
scale that getPixels did not support. We already have a test for that in
CodexTest (although it only tests one image).

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:425: // (stronger downscalings are fine). In this case,
returns the non-scaled version.
On 2016/01/21 10:43:30, adaubert wrote:
> On 2016/01/20 21:42:20, yujieqin wrote:
> > Just for clarification, does it mean we can keep our current solution for
now?
> > :)
> 
> Downscaling to a smaller size, in this case 1/3, would be Ok. Like Yujie said,
> it's up to you guys what you would prefer.

My preference would be 1/3 for now. The caller can always decide that that's too
small. Thinking about it now, telling the caller they can decode to full size is
probably never interesting - they should always assume that is an option.

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:436: bool SkRawCodec::onDimensionsSupported(const
SkISize& dim) {
On 2016/01/20 21:42:20, yujieqin wrote:
> * The image in current CL is not xtrans.
> * I think more tests is always good. Could we do that after this CL? I think
if
> the CodexTest I changed is working, we may first rely on that?

I think that's a fine start.

https://codereview.chromium.org/1520403003/diff/430001/DEPS
File DEPS (right):

https://codereview.chromium.org/1520403003/diff/430001/DEPS#newcode25
DEPS:25: "third_party/externals/dng_sdk" :
"https://android.googlesource.com/platform/external/dng_sdk.git@master",
I just realized that this points to a branch rather than a particular commit or
tag. All of our other DEPS use a particular tag. Sticking with a particular
commit or tag has the downside of meaning that we need to change this file to
get new commits (e.g. your fix for mac), but it also means things won't suddenly
(and mysteriously, to a Skia developer) stop working if someone changed
upstream. For example, if someone landed a change in upstream that required a
makefile change, we need to update our gyp file. But if we silently get the new
version, it will take some extra work (for someone who probably isn't familiar
with the library) to figure out what gyp file changes are necessary.

Anyway, please specify the commit to use for both libraries.

https://codereview.chromium.org/1520403003/diff/430001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/430001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:326: // FIXME: the ::piex::GetPreviewImageData() may
"reallocate" the SkRawStream->fStreamBuffer
nit: Looking at the implementation of SkDynamicMemoryWStream, it looks like it
wisely has a minimum size per block in its linked list (256), so a two byte read
won't require its own allocation (whew!). The only time I think this would
potentially be wasteful allocation-wise is if we allocate two blocks separately
of over 256 bytes that could've been one. We also do some extra work in safety
checking, though. Not sure whether that's a real issue. If it is, we could
change bufferMoreData to buffer at least the amount requested (or piex to
request how much it will need in larger blocks). But we have a conflicting goal,
which is to only read as much of the stream as is necessary. Maybe that's okay,
though, since reading "too much" meant that it is not a RAW image, and we are
not going to do anything with the stream afterwards. (The goal for reading less
is that the caller can buffer less while learning the size, after which they'll
rewind and decode the full image. If it's not an image, they won't be rewinding
and reading again. Further, I presume attempting to decode arbitrary non-image
streams is not a common use case.)

yujieqin

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp File src/codec/SkCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkCodec.cpp#newcode94 src/codec/SkCodec.cpp:94: if (streamDeleter) { Thanks for the tips. I got ...

4 years, 11 months ago (2016-01-21 16:40:01 UTC) #68

msarett

https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/410001/src/codec/SkRawCodec.cpp#newcode330 src/codec/SkRawCodec.cpp:330: if (error == ::piex::Error::kOk && imageData.preview_length > 0) { ...

4 years, 11 months ago (2016-01-21 16:48:49 UTC) #69

scroggo

https://codereview.chromium.org/1520403003/diff/470001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/470001/src/codec/SkRawCodec.cpp#newcode365 src/codec/SkRawCodec.cpp:365: SkSwizzler::kRGB, nullptr, requestedInfo, options)); SkASSERT(swizzler)?

4 years, 11 months ago (2016-01-21 17:36:08 UTC) #70

yujieqin

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp#newcode107 src/codec/SkRawCodec.cpp:107: if (safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) { En... good to know. ...

4 years, 11 months ago (2016-01-21 17:44:26 UTC) #71

scroggo

Some small nits, but otherwise LGTM. In order to submit, there should be a checkbox ...

4 years, 11 months ago (2016-01-21 17:57:43 UTC) #72

Some small nits, but otherwise LGTM.

In order to submit, there should be a checkbox in codereview that says "Commit".
This will run it through our queue which tests it on a bunch of different
platforms (I think mostly to make sure it builds).

Make sure you click that checkbox when you can be available for a while to fix
any problems that might come up. We may have to revert anyway, but that will
help us make sure it lands and sticks.

Separately I'll get started on another merge into Android's master branch so
Android can start using it.

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/370001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:107: if (safe_add_to_size_t(alreadyBuffered, bytesRead,
&newSize)) {
On 2016/01/21 17:44:25, yujieqin wrote:
> En... good to know.
> 
> Could we maybe leave it as it is, and could you help to create a skia bug to
> track the progress of improving the behavior of SkStream.read() for large size
> case? As you mentioned, someone (kjlubick@) is already working on something
> related, right?

Well, he's just added code to "fuzz" our image decoders, meaning it will modify
existing image files and try decoding them to try to hit error cases. So it's
not aimed at SkStream::read() directly, but it should help.

https://codereview.chromium.org/1520403003/diff/490001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1520403003/diff/490001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:326: // FIXME: ::piex::GetPreviewImageData() calls read
(or whatever function it calls) frequently
Instead of "read (or whatever function it calls)", I think the function it calls
is "GetData()"? Please say that instead.

https://codereview.chromium.org/1520403003/diff/490001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:328: // efficient by requesting more than needed?
Sorry, I know this is what I suggested, but I think it could still be clearer:

"... efficient by grouping smaller requests together."

yujieqin

https://codereview.chromium.org/1520403003/diff/490001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1520403003/diff/490001/src/codec/SkRawCodec.cpp#newcode326 src/codec/SkRawCodec.cpp:326: // FIXME: ::piex::GetPreviewImageData() calls read (or whatever function it ...

4 years, 11 months ago (2016-01-21 18:06:12 UTC) #73

yujieqin

Description was changed from ========== Prototype of RAW decoding in Skia. Note: * Need to ...

4 years, 11 months ago (2016-01-21 18:11:46 UTC) #75

yujieqin

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the ...

4 years, 11 months ago (2016-01-21 18:12:26 UTC) #77

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/510001

4 years, 11 months ago (2016-01-21 18:12:33 UTC) #78

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-21 18:13:17 UTC) #79

commit-bot: I haz the power

Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot/builds/5410) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, ...

4 years, 11 months ago (2016-01-21 18:13:18 UTC) #80

scroggo

When I run DM on my mac laptop using the images you provided, I get ...

4 years, 11 months ago (2016-01-21 23:31:35 UTC) #81

adaubert

On 2016/01/21 23:31:35, scroggo wrote: > When I run DM on my mac laptop using ...

4 years, 11 months ago (2016-01-22 12:33:24 UTC) #82

On 2016/01/21 23:31:35, scroggo wrote:
> When I run DM on my mac laptop using the images you provided, I get the
> following failures:
> 
> Failures:
> 	8888 image decode Samsung_NX3000.SRW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Samsung_NX3000.SRW.
> 	8888 image decode Pentax_K5.PEF: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Pentax_K5.PEF.
> 	8888 image decode Nikon_P330.NRW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Nikon_P330.NRW.
> 	8888 image decode Sony_RX100_III.ARW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Sony_RX100_III.ARW.
> 	565 image decode Samsung_NX3000.SRW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Samsung_NX3000.SRW.
> 	565 image decode Pentax_K5.PEF: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Pentax_K5.PEF.
> 	565 image decode Nikon_P330.NRW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Nikon_P330.NRW.
> 	565 image decode Sony_RX100_III.ARW: Couldn't decode
> /Users/scroggo/Downloads/Skia_RAW_image_set/Sony_RX100_III.ARW.
> 
> Should these images succeed? If not, I'll need to remove them from the set
we're
> going to test.
> 
> Over chat, yujieqin@ said:
> > After one more rebase, the DM tool failed with something like:
> > ( 340/1510MB    247) 7.53ms     8888 image decode
> IMG_0265.CR2../../dm/DM.cpp:857: failed assertion
> > "bitmap.copyTo(&swizzle, kRGBA_8888_SkColorType)"
> > Segmentation fault (core dumped)
> 
> I did not see that when I ran it, but one possibility is OOM. copyTo can fail
> for other reasons e.g. bad color types, but the color types look good, and I
do
> not see any obvious reason it should fail.
copyTo failed in this case because of bitmap.info().dimensions().IsEmpty(). more
see below.

We haven't developed a dedicated SkImageDecoder for RAWs, like it is available
for JPEG, PNG and so on. In such a case, on mac, it falls back to
SkImageDecoder_CG, which supports some RAW formats but not all. For the one
which it does not support, SkImageDecoder_CG returns an error, and exactly this
is the case for the mentioned images (plus also these: Olympus_E-PL3.ORF,
Olympus_PL7.ORF, Panasonic_GM5.RW2).

On Linux (the platform where Yujie tested), there seems to be no fallback
available. So it fails for all RAW images. There is also an error reported at
https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=819, but since there is no
return, it will continue. Since there was no SkImageDecoder created, the bitmap
has an empty size, but with the kBGRA_8888_SkColorType colortype. With that it
tries to copy (convert) the image at
https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=855 and fails, since it can't
lock an image with an dimensions of size 0.
(BTW: I think we should "return" at
https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=819 or if the
bitmap.info().dimensions().IsEmpty())

Anyway, this SkImageDecoder seems to be not relevant for our goal to bring RAW
support on Android, right? At least it works fine on Android N.
So, then the question is how to make the testing tools happy. Removing the
mentioned images would only help on Mac, but would still let linux and others
fail.
Any ideas how we short proceed short term?
in_reply_to: 5718016125829120
send_mail: 1
subject: Prototype of RAW decoding in Skia.

msarett

Yes, SkImageDecoder support is completely irrelevant! We are replacing SkImageDecoder with SkCodec and intend to ...

4 years, 11 months ago (2016-01-22 14:20:50 UTC) #83

scroggo

On 2016/01/22 12:33:24, adaubert wrote: > On 2016/01/21 23:31:35, scroggo wrote: > > When I ...

4 years, 11 months ago (2016-01-22 14:23:00 UTC) #84

On 2016/01/22 12:33:24, adaubert wrote:
> On 2016/01/21 23:31:35, scroggo wrote:
> > When I run DM on my mac laptop using the images you provided, I get the
> > following failures:
> > 
> > Failures:
> > 	8888 image decode Samsung_NX3000.SRW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Samsung_NX3000.SRW.
> > 	8888 image decode Pentax_K5.PEF: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Pentax_K5.PEF.
> > 	8888 image decode Nikon_P330.NRW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Nikon_P330.NRW.
> > 	8888 image decode Sony_RX100_III.ARW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Sony_RX100_III.ARW.
> > 	565 image decode Samsung_NX3000.SRW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Samsung_NX3000.SRW.
> > 	565 image decode Pentax_K5.PEF: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Pentax_K5.PEF.
> > 	565 image decode Nikon_P330.NRW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Nikon_P330.NRW.
> > 	565 image decode Sony_RX100_III.ARW: Couldn't decode
> > /Users/scroggo/Downloads/Skia_RAW_image_set/Sony_RX100_III.ARW.
> > 
> > Should these images succeed? If not, I'll need to remove them from the set
> we're
> > going to test.
> > 
> > Over chat, yujieqin@ said:
> > > After one more rebase, the DM tool failed with something like:
> > > ( 340/1510MB    247) 7.53ms     8888 image decode
> > IMG_0265.CR2../../dm/DM.cpp:857: failed assertion
> > > "bitmap.copyTo(&swizzle, kRGBA_8888_SkColorType)"
> > > Segmentation fault (core dumped)
> > 
> > I did not see that when I ran it, but one possibility is OOM. copyTo can
fail
> > for other reasons e.g. bad color types, but the color types look good, and I
> do
> > not see any obvious reason it should fail.
> copyTo failed in this case because of bitmap.info().dimensions().IsEmpty().
more
> see below.
> 
> 
> We haven't developed a dedicated SkImageDecoder for RAWs, like it is available
> for JPEG, PNG and so on. In such a case, on mac, it falls back to
> SkImageDecoder_CG, which supports some RAW formats but not all. For the one
> which it does not support, SkImageDecoder_CG returns an error, and exactly
this
> is the case for the mentioned images (plus also these: Olympus_E-PL3.ORF,
> Olympus_PL7.ORF, Panasonic_GM5.RW2).
> 
> On Linux (the platform where Yujie tested), there seems to be no fallback
> available. So it fails for all RAW images. There is also an error reported at
> https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=819, but since there is no
> return, it will continue. Since there was no SkImageDecoder created, the
bitmap
> has an empty size, but with the kBGRA_8888_SkColorType colortype. With that it
> tries to copy (convert) the image at
> https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=855 and fails, since it
can't
> lock an image with an dimensions of size 0.
> (BTW: I think we should "return" at
> https://cs.corp.google.com/#skia/skia/dm/DM.cpp&l=819 or if the
> bitmap.info().dimensions().IsEmpty())
> 
> Anyway, this SkImageDecoder seems to be not relevant for our goal to bring RAW
> support on Android, right? At least it works fine on Android N.
> So, then the question is how to make the testing tools happy. Removing the
> mentioned images would only help on Mac, but would still let linux and others
> fail.
> Any ideas how we short proceed short term?
> in_reply_to: 5718016125829120
> send_mail: 1
> subject: Prototype of RAW decoding in Skia.

Ah, of course. I forgot that "decode" in that line meant SkImageDECODEr. It is
fine that that is failing. We're going to remove SkImageDecoder anyway. In the
short term, I can disable it for RAW. Feel free to try to commit again.

scroggo

On 2016/01/22 14:20:50, msarett wrote: > Yes, SkImageDecoder support is completely irrelevant! We are replacing ...

4 years, 11 months ago (2016-01-22 14:28:59 UTC) #85

yujieqin

https://codereview.chromium.org/1520403003/diff/530001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1520403003/diff/530001/dm/DM.cpp#newcode534 dm/DM.cpp:534: push_src("image", "decode", new ImageSrc(path)); // Decode entire image On ...

4 years, 11 months ago (2016-01-22 20:18:08 UTC) #86

yujieqin

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to ...

4 years, 11 months ago (2016-01-22 20:18:50 UTC) #88

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/550001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/550001

4 years, 11 months ago (2016-01-22 20:19:03 UTC) #89

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-22 20:20:15 UTC) #90

commit-bot: I haz the power

Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/builds/5619)

4 years, 11 months ago (2016-01-22 20:20:17 UTC) #91

yujieqin

Hi Leon & Matt, could you give me lgtm again for the new patch set? ...

4 years, 11 months ago (2016-01-22 20:23:22 UTC) #92

msarett

Description was changed from ========== Add RAW decoding into Skia. BUG=skia: (Based on the work ...

4 years, 11 months ago (2016-01-22 20:26:14 UTC) #93

yujieqin

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

4 years, 11 months ago (2016-01-22 20:30:28 UTC) #94

msarett

msarett@google.com changed reviewers: - reed@google.com

4 years, 11 months ago (2016-01-22 20:31:41 UTC) #95

msarett

I've added TBR=reed@google.com, which should help on the skia_presubmit_bot. There are others errors as well. ...

4 years, 11 months ago (2016-01-22 20:31:42 UTC) #96

yujieqin

Thanks, I also just added reed@google.com to reviewer as mentioend in the error log. :) ...

4 years, 11 months ago (2016-01-22 20:37:41 UTC) #97

msarett

Every time you click commit, all of the trybots will run. It looks like there ...

4 years, 11 months ago (2016-01-22 20:40:55 UTC) #98

yujieqin

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to ...

4 years, 11 months ago (2016-01-22 20:42:41 UTC) #101

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/570001

4 years, 11 months ago (2016-01-22 20:42:49 UTC) #102

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-22 20:43:53 UTC) #103

commit-bot: I haz the power

Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot/builds/942)

4 years, 11 months ago (2016-01-22 20:43:55 UTC) #104

adaubert

The CQ bit was checked by adaubert@google.com to run a CQ dry run

4 years, 11 months ago (2016-01-25 08:16:33 UTC) #105

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/570001

4 years, 11 months ago (2016-01-25 08:16:41 UTC) #106

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-25 08:17:58 UTC) #107

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/5566) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on ...

4 years, 11 months ago (2016-01-25 08:17:59 UTC) #108

yujieqin

The CQ bit was checked by yujieqin@google.com to run a CQ dry run

4 years, 11 months ago (2016-01-25 12:46:26 UTC) #109

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/590001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/590001

4 years, 11 months ago (2016-01-25 12:46:38 UTC) #110

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-25 12:49:03 UTC) #111

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/5576)

4 years, 11 months ago (2016-01-25 12:49:04 UTC) #112

yujieqin

The CQ bit was checked by yujieqin@google.com to run a CQ dry run

4 years, 11 months ago (2016-01-25 13:06:59 UTC) #113

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/610001

4 years, 11 months ago (2016-01-25 13:07:06 UTC) #114

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 11 months ago (2016-01-25 13:25:02 UTC) #115

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 11 months ago (2016-01-25 13:25:03 UTC) #116

yujieqin

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to ...

4 years, 11 months ago (2016-01-25 13:28:04 UTC) #118

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/610001

4 years, 11 months ago (2016-01-25 13:28:10 UTC) #119

commit-bot: I haz the power

Description was changed from ========== Add RAW decoding into Skia. TBR=reed@google.com BUG=skia: (Based on the ...

4 years, 11 months ago (2016-01-25 13:29:06 UTC) #120

commit-bot: I haz the power

Committed patchset #32 (id:610001) as https://skia.googlesource.com/skia/+/6bd8639f8c142eedf543f4e5f3b02d2bf11df308

4 years, 11 months ago (2016-01-25 13:29:08 UTC) #121

msarett

A revert of this CL (patchset #32 id:610001) has been created in https://codereview.chromium.org/1635443002/ by msarett@google.com. ...

4 years, 11 months ago (2016-01-25 14:04:59 UTC) #122

msarett

Description was changed from ========== Add RAW decoding into Skia. TBR=reed@google.com BUG=skia: (Based on the ...

4 years, 11 months ago (2016-01-25 14:37:23 UTC) #123

yujieqin

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to ...

4 years, 11 months ago (2016-01-25 16:14:16 UTC) #125

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1520403003/690001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1520403003/690001

4 years, 11 months ago (2016-01-25 16:14:22 UTC) #126

dogben

On 2016/01/25 at 14:04:59, msarett wrote: > A revert of this CL (patchset #32 id:610001) ...

4 years, 11 months ago (2016-01-25 16:16:00 UTC) #127

commit-bot: I haz the power

Description was changed from ========== Add RAW decoding into Skia. TBR=reed@google.com BUG=skia: (Based on the ...

4 years, 11 months ago (2016-01-25 16:26:19 UTC) #128

commit-bot: I haz the power

4 years, 11 months ago (2016-01-25 16:26:21 UTC) #129

Message was sent while issue was closed.

Committed patchset #36 (id:690001) as
https://skia.googlesource.com/skia/+/916de9ff18cf3caa29c0821b55244060b6f84f9d

Issue 1520403003: Prototype of RAW decoding in Skia. (Closed)

Description

Patch Set 1 : Initial upload of the Prototype. #

Patch Set 2 : Addrees comments from current & old CLs #

Patch Set 3 : Address new comments #

Patch Set 4 : Merge the change for DEPS #

Patch Set 5 : Implement SkRawStream. Fixed new comments #

Patch Set 6 : * Add DNG scaling. * Adress all comments. #

Patch Set 7 : Address comments #

Patch Set 8 : Address comments #

Patch Set 9 : Revert unnecessary changes #

Patch Set 10 : Address Comments #

Patch Set 11 : Change the scaling logic for DNG. Address comments. #

Patch Set 12 : Improve render(). Address comments. #

Patch Set 13 : Change the DEPS for PIEX to use AOSP #

Patch Set 14 : Small fix on render() #

Patch Set 15 : Add test, address comments #

Patch Set 16 : More comments. #

Patch Set 17 : Fix .gyp(s) #

Patch Set 18 : Fix the TODO for XTrans images at scale > 0.5. #

Patch Set 19 : Small fix on code #

Patch Set 20 : Small fix #

Patch Set 21 : Address comments #

Patch Set 22 : Fix bug in transferBuffer. #

Patch Set 23 : Address comments #

Patch Set 24 : Rebase the code #

Patch Set 25 : Address comments solved by rebase #

Patch Set 26 : Address comments #

Patch Set 27 : Address comments #

Patch Set 28 : Rebase #

Patch Set 29 : Fix DM #

Patch Set 30 : Fix test #

Patch Set 31 : Fix test and fix the multiple calls for onGetPixels #

Patch Set 32 : Fix compile issues #

Patch Set 33 : Fix compiler issues #

Patch Set 34 : Disable ::posix_memalign() in DNG SDK #

Patch Set 35 : Disable ::posix_memalign() in gyp/dng_sdk.gyp #

Patch Set 36 : Set big endian explicitly for arm #

Messages