Report repetition count in SkCodec

include/codec/SkCodec.h

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkCodec_DEFINED
 #define SkCodec_DEFINED
 
@@ skipping 586 matching lines @@
          *  kNone.
          */
         size_t fRequiredFrame;
 
         /**
          *  Number of milliseconds to show this frame.
          */
         size_t fDuration;
     };
 
+    static constexpr int kRepetitionCountInfinite = -1;
+
     /**
      *  Return info about the frames in the image.
      *
      *  May require reading through the stream to determine the number of
      *  frames.
      *
      *  As such, future decoding calls may require a rewind.
      *
-     *  For single-frame images, this will return an empty vector.
+     *  For single-frame images, this will return an empty vector and leave
+     *  outRepetitionCount unmodified.
+     *
+     *  @param outRepetitionCount If non-NULL, this will be set to the number of
+     *      times an animated image should repeat.
      */
-    std::vector<FrameInfo> getFrameInfo() {
-        return this->onGetFrameInfo();
+    std::vector<FrameInfo> getFrameInfo(int* outRepetitionCount = nullptr) {

[cblume, 2016/10/26 17:45:59]

I like where this is going.

I am pretty sure I would always get the repetition count when calling this. Is
there a use case where we wouldn't? Maybe because we call this multiple times
when we provide more buffers?

If we would always specify outRepetitionCount (btw I love "out" in the name),
1.) we don't need a default value, but more importantly
2.) maybe we should be returning a struct that contains the vector and the
repetition count

[scroggo_chromium, 2016/10/26 18:42:18]

(I assume "more buffers" means "more data"?) Yes, I suppose that if the first
call revealed the repetition count, and then you supplied more data, you would
not need to query the repetition count again, since it should not change. On the
other hand, it's probably not very expensive to query it a second time, since
we've already cached the value.

Maybe that justifies reporting a different value for "we have not seen the loop
count" yet, but I think it just means "do not loop" (unless we later change our
minds), which is the same as a zero returned by the decoder.
I guess I just imagine the repetition count to be less interesting than the
FrameInfo. I cannot imagine someone wanting the repetition count without the
FrameInfo, but I *could* imagine someone wanting the FrameInfo without the
repetition count (e.g. because they want to continue to loop regardless).

Another option that fits with Skia conventions - return a boolean (i.e. whether
this image has multiple frames) and use out-parameters for the repetition count
and the vector (either in a single struct or as separate parameters).

I don't think any of these options put restrictions on the client that should be
a problem, so I'll defer to reed@ on what is most appropriate for our API.

[scroggo_chromium, 2016/10/27 20:54:16]

patch set 2 returns a bool, and uses multiple out parameters. I kind of like
patch set 1, because I can say:

auto frameInfo = codec->getFrameInfo(&repetitionCount)

which saves a little bit of typing. But I don't have a strong preference.

[cblume, 2016/10/28 01:09:41]

patch set 2's solution would allow us to reuse allocated space. However, I don't
expect to be calling this function so many times that this would matter. I agree
that patch set 1 reads more easily and feels more natural. I like it, but I'm
okay with either.

[cblume, 2016/10/28 01:09:41]

Actually, I may have said this wrong.

I definitely want to know the repetition count at some point. But if I am making
multiple calls to getFrameInfo (as I provide more data), I don't need it each
time. I only really need it once I have provided all the data. I only ever need
to get it once.

In that way, would it be better to keep the concepts separate? The frameInfo is
different from the repetition count. Should they be separate functions?

[scroggo_chromium, 2016/10/28 11:49:40]

I still think you always want the FrameInfo when you want the repetition count,
but you never want the repetition count without the FrameInfo. (Maybe the client
will want to find them out at different times architecturally, but logically I
think you need to query them both once you've supplied all the data, since the
final bit of data may have more frames and the count.)

I'm not opposed to splitting them up, but I mainly kept them together because I
felt it was consistent with the decision we made for getFrameInfo. I originally
had one method to query the number of frames, and then other methods to query
the duration and dependent frame information, but we decided to provide a single
query that provided all of the information. Just like the repetition count,
after receiving the information about the first frame, you don't need to query
that again, but we still provide it each time you provide more data and ask for
the frame info.

+        return this->onGetFrameInfo(outRepetitionCount);
     }
 
 protected:
     /**
      *  Takes ownership of SkStream*
      */
     SkCodec(int width,
             int height,
             const SkEncodedInfo&,
             SkStream*,
@@ skipping 125 matching lines @@
      *
      *  Returns -1 if we have not started a scanline decode.
      */
     int currScanline() const { return fCurrScanline; }
 
     virtual int onOutputScanline(int inputScanline) const;
 
     bool initializeColorXform(const SkImageInfo& dstInfo);
     SkColorSpaceXform* colorXform() const { return fColorXform.get(); }
 
-    virtual std::vector<FrameInfo> onGetFrameInfo() {
+    virtual std::vector<FrameInfo> onGetFrameInfo(int*) {
         // empty vector - this is not animated.
         return {};
     }
 
     /**
      *  Used for testing with qcms.
      *  FIXME: Remove this when we are done comparing with qcms.
      */
     virtual sk_sp<SkData> getICCData() const { return nullptr; }
 
@@ skipping 76 matching lines @@
     // For testing with qcms
     // FIXME: Remove these when we are done comparing with qcms.
     friend class DM::ColorCodecSrc;
     friend class ColorCodecBench;
 
     friend class DM::CodecSrc;  // for fillIncompleteImage
     friend class SkSampledCodec;
     friend class SkIcoCodec;
 };
 #endif // SkCodec_DEFINED