Destroy SkScanlineDecoder in the SkCodec subclass destructors

In the case of subset decodes, we will often not decode to
the bottom of the image.  In our code before this patch,
this would result in cleanup code in onFinish() never being
called.

We can allow subclasses to take ownership of the
SkScanlineDecoder in order to make sure that it is
finished/deleted before deleting the decode manager.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/c0e80c139e15496a8a96eec7848689b6f0e7bcc1

[msarett, 2015-06-29 21:17:16]

msarett@google.com changed reviewers:
+ emmaleer@google.com, reed@google.com, scroggo@google.com

[reed1, 2015-06-29 21:21:49]

https://codereview.chromium.org/1212593003/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1212593003/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:182: * Get method for the scanline decoder
nit: lets have the comment be slightly more useful, or eliminate it.

e.g.

/* If the codec has a scanline decoder, return it (no ownership change occurs)
else return NULL.
   The returned decoder is only valid until ...
*/

[scroggo, 2015-06-29 21:33:22]

Please add tests. (If it is convenient, you can add them to tests/CodexTest.cpp,
or you can add a new file.)

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:302: fDecoderMgr->returnFailure("cannot getPixels() if
a scanline decoder has been created",
I think this should be:

    return fDecoderMgr->...

?

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:308: fDecoderMgr->returnFailure("could not rewind
stream", kCouldNotRewind);
Again, this should be a return statement, right?

[msarett, 2015-06-29 21:59:43]

The test I added makes sure that we return an error when a regular decode is
attempted after creating a scanline decoder.  This would catch me forgetting to
type "return".

As far as testing that finish() is always called and called only once, I think
our best bet is dm and ASAN (which is how I noticed this issue in the first
place).

https://codereview.chromium.org/1212593003/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1212593003/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:182: * Get method for the scanline decoder
On 2015/06/29 21:21:49, reed1 wrote:
> nit: lets have the comment be slightly more useful, or eliminate it.
> 
> e.g.
> 
> /* If the codec has a scanline decoder, return it (no ownership change occurs)
> else return NULL.
>    The returned decoder is only valid until ...
> */

Done.

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:302: fDecoderMgr->returnFailure("cannot getPixels() if
a scanline decoder has been created",
On 2015/06/29 21:33:22, scroggo wrote:
> I think this should be:
> 
>     return fDecoderMgr->...
> 
> ?

Yeah it should be!

https://codereview.chromium.org/1212593003/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:308: fDecoderMgr->returnFailure("could not rewind
stream", kCouldNotRewind);
On 2015/06/29 21:33:22, scroggo wrote:
> Again, this should be a return statement, right?

Yep this is wrong too.  That's what you get when you copy and paste incorrect
code.

[scroggo, 2015-06-30 13:47:38]

https://codereview.chromium.org/1212593003/diff/20001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1212593003/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:21: this->onFinish();
I almost missed this: you shouldn't call a virtual function from a destructor.
(See http://www.artima.com/cppsource/nevercall.html)

Go ahead and remove onFinish, and put its implementation in destructor
overrides.

[scroggo, 2015-06-30 14:50:17]

https://codereview.chromium.org/1212593003/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1212593003/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:520: return kInvalidInput;
This is intended to mean that the encoded data is invalid. In this case, it was
an incorrect use by the caller. SkImageGenerator::kInvalidParameters seems like
the error code that most closely matches the error (or maybe it should be a new
code?).

[msarett, 2015-06-30 20:30:46]

Patchset #3 (id:40001) has been deleted

[msarett, 2015-06-30 20:43:05]

This CL has changed quite a bit due to problems that came up in the last round.

The problem is that we need the finish() routine (or equivalent code) for the
SkScanlineDecoder to be called before the codec subclass (SkJpegCodec,
SkPngCodec) is destroyed.  Since the SkScanlineDecoder is owned by the base
class, we cannot accomplish this by overriding the SkScanlineDecoder destructor.

Leon and I discussed options:
1) Always manually call finish() after completing a scanline decode.
2) Make SkScanlineDecoder owned by the codec subclass.
3) Make the JpegDecoderMgr and PngDecoderMgr into a generic DecoderMgr that is
owned by SkCodec.  I think that this may be possible with tricky/clever
refactoring, but IMO it makes more sense to keep these separate.

4) This is what I implemented.  By adding a protected detachScanlineDecoder()
function to SkCodec, the subclass can take ownership of the SkScanlineDecoder
during destruction and make sure everything happens in the correct order.

This is a little weird because 2/2 of our scanline decoders use this strategy. 
The SkScanlineDecoder is never deleted by the base class.  I have no idea if
future SkScanlineDecoders will need to be handled this way as well, or if the
original strategy of ownership by the base class will eventually be fully
utilized.

I like this approach anyway because it greatly reduces the amount of duplicated
code as compared to (2).

https://codereview.chromium.org/1212593003/diff/20001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1212593003/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:21: this->onFinish();
On 2015/06/30 13:47:38, scroggo wrote:
> I almost missed this: you shouldn't call a virtual function from a destructor.
> (See http://www.artima.com/cppsource/nevercall.html)
> 
> Go ahead and remove onFinish, and put its implementation in destructor
> overrides.

Acknowledged.

https://codereview.chromium.org/1212593003/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1212593003/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:520: return kInvalidInput;
On 2015/06/30 14:50:17, scroggo wrote:
> This is intended to mean that the encoded data is invalid. In this case, it
was
> an incorrect use by the caller. SkImageGenerator::kInvalidParameters seems
like
> the error code that most closely matches the error (or maybe it should be a
new
> code?).

I agree, InvalidParameters makes more sense.

[scroggo, 2015-06-30 20:59:58]

lgtm

You'll still need mike's approval for API change.

https://codereview.chromium.org/1212593003/diff/60001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1212593003/diff/60001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:32: virtual ~SkScanlineDecoder() {}
Do any clients override this? Maybe we can leave it out (and leave the implicit,
non-virtual destructor).

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

https://codereview.chromium.org/1212593003/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:404: SkCodecPrintf("setjmp: Error in libjpeg
finish_decompress\n");
Do we need this error message? I guess it could be a sign of a deeper problem,
but it seems like we have already decoded the image?

[msarett, 2015-06-30 21:33:00]

https://codereview.chromium.org/1212593003/diff/60001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1212593003/diff/60001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:32: virtual ~SkScanlineDecoder() {}
On 2015/06/30 20:59:58, scroggo wrote:
> Do any clients override this? Maybe we can leave it out (and leave the
implicit,
> non-virtual destructor).

No clients override this.  My original response was to agree to remove this. 
But with this removed we get:

error: ‘class SkScanlineDecoder’ has virtual functions and accessible
non-virtual destructor [-Werror=non-virtual-dtor]

The internet tells me this is bad because if we wrote:
BaseClass obj = new SubClass();
delete obj;
The call to delete would not be able to call the destructor of the SubClass. 
This is not an issue for us since none of our subclasses have destructors.

I still think it's better to avoid the warning and keep it virtual.  We do in
fact need a public destructor in order to use SkAutoTDeletes.  I guess maybe we
will actually want to override it in a subclass sometime.

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

https://codereview.chromium.org/1212593003/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:404: SkCodecPrintf("setjmp: Error in libjpeg
finish_decompress\n");
On 2015/06/30 20:59:58, scroggo wrote:
> Do we need this error message? I guess it could be a sign of a deeper problem,
> but it seems like we have already decoded the image?

I would lean towards keeping it.  It doesn't print under default settings, and
I'd like to be warned if somehow we aren't using the jpeg library properly.  We
may not care, but I'd like to know something is "wrong" and make sure it's not
really wrong.

That said this is not a huge issue and if you feel more strongly than me, I'll
drop it :).

[reed1, 2015-07-01 13:37:34]

api lgtm

I have broader questions, but not specific to this CL

[msarett, 2015-07-01 13:43:31]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-07-01 13:43:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212593003/60001

[commit-bot: I haz the power, 2015-07-01 13:50:40]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/c0e80c139e15496a8a96eec7848689b6f0e7bcc1

[scroggo, 2015-07-01 14:08:44]

LGTM as is