sk_at_scope_end

src/codec/SkWebpCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodecPriv.h"
 #include "SkWebpCodec.h"
 #include "SkTemplates.h"
@@ skipping 90 matching lines @@
     // FIXME (msarett):
     // Temporary strategy for getting ICC profiles from webps.  Once the incremental decoding
     // API lands, we will use the WebPDemuxer to manage the entire decode.
     sk_sp<SkColorSpace> colorSpace = nullptr;
     const void* memory = stream->getMemoryBase();
     if (memory) {
         WebPData data = { (const uint8_t*) memory, stream->getLength() };
         WebPDemuxState state;
         SkAutoTCallVProc<WebPDemuxer, WebPDemuxDelete> demux(WebPDemuxPartial(&data, &state));
 
         WebPChunkIterator chunkIterator;

[bungeman-skia, 2016/08/24 21:39:49]

Now that we've established webp has done a silly thing here, this seems more
like it wants unique_resource from p0052 instead of scope_exit. In other words,
it would be nice if the type itself managed the lifetime, instead of some
external guard. Something like

unique_resource<WebPChunkIterator, [](WebPChunkIterator&
r){WebPDemuxReleaseChunkIterator(&r)}> chunkIterator;

Ok, so maybe that doesn't look quite as nice, but that's mostly because we want
a value type instead of a handle type. A generic adapter for that sort of thing
could look like

template <typename T, typename D> using unique_value = unique_resource<T, [](T&
t){D(&t)}>;
unique_value<WebPChunkIterator, WebPDemuxReleaseChunkIterator> chunkIterator;

Note that all these variations here should turn into the same machine code, but
with something like 'unique_value' I can see at a glance from the type that this
is a scoped value of a given type and a specified thing will run on it when it
goes out of scope. Spreading this out over two statements seems like more mental
overhead.

-        SkAutoTCallVProc<WebPChunkIterator, WebPDemuxReleaseChunkIterator> autoCI(&chunkIterator);
+        sk_at_scope_end(WebPDemuxReleaseChunkIterator(&chunkIterator));
         if (demux && WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) {

[bungeman-skia, 2016/08/24 15:18:17]

Is it valid to call WebPDemuxReleaseChunkIterator on an uninitialized
WebPChunkIterator? If demux is false (and I'm assuming it can be, since we're
testing it here), then WebPDemuxGetChunk is never called to initialize
chunkIterator, and even if it is, WebPDemuxGetChunk can fail and again
chunkIterator isn't called.

Is there a need for this scoped thing at all? shouldn't
WebPDemuxReleaseChunkIterator be called on chunkIterator only inside this 'if'
block right after calling SkColorSpace::NewICC?

[mtklein, 2016/08/24 16:52:58]

Gonna have to leave this one to Matt.
I don't think this CL changes anything related to this question.

[msarett, 2016/08/24 17:01:25]

I'm following the same pattern as chromium here.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

IMO, it makes sense for WebPDemuxReleaseChunkIterator to not fail even if we
haven't managed to read any chunks.  That's the behavior we get from
jpeg_destory() etc.

Amusingly, this function actually does nothing anyway, so it seems that it
doesn't matter.
https://cs.chromium.org/chromium/src/third_party/libwebp/demux/demux.c?sq=pac...

I think this is fine as is.  If we decide we want to fix, I'll handle it in a
follow-up CL.

             colorSpace = SkColorSpace::NewICC(chunkIterator.chunk.bytes, chunkIterator.chunk.size);
         }
     }
 
     if (!colorSpace) {
         colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
     }
 
     SkEncodedInfo info = SkEncodedInfo::Make(color, alpha, 8);
     return new SkWebpCodec(features.width, features.height, info, colorSpace,
@@ skipping 162 matching lines @@
             default:
                 return kInvalidInput;
         }
     }
 }
 
 SkWebpCodec::SkWebpCodec(int width, int height, const SkEncodedInfo& info,
                          sk_sp<SkColorSpace> colorSpace, SkStream* stream)
     : INHERITED(width, height, info, stream, colorSpace)
 {}