sk_at_scope_end

src/codec/SkWebpCodec.cpp

Index: src/codec/SkWebpCodec.cpp
diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp
index c28d077bb329826750ebca4bb92ac2f005e2ac02..21cf598419a77148004c83689c9483ac14a56807 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp
@@ -109,7 +109,7 @@ SkCodec* SkWebpCodec::NewFromStream(SkStream* stream) {
         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);
         }