Leon, wanna take this issue?

What is the goal here? I think the information is interesting, but what will be done with it? (Is this a standard part of adding a feature?)

I don't see way to count the raw number (which probably doesn't make any sense anyway), so I suppose you want to count relative to something else? I considered adding another DecodedImageType, but that would (presumably) split PNG into two enums, which might be confusing when tracking across my change. (I suppose we could double count APNG as PNG and APNG? That would change the overall percentages, though.)

Do you mean to count relative to static PNGs? I uploaded https://codereview.chromium.org/2800193002 to do just that. (It doesn't go through BitmapImage::sizeAvailable, which would require plumbing isAnimated up through DeferredImageDecoder and ImageSource; instead it happens inside PNGImageReader, when it reports the size, so no API changes are necessary.)

I think it might be interesting to track relative use of animated GIF versus animated PNG versus animated WEBP, but it's not obvious to me how to do that. While PNG and WEBP know whether they are animated once we've decoded the size, GIF does not know until later (once a second frame has been received and parsed). I guess we could delay the checks for PNG and WEBP?

> (It doesn't go through BitmapImage::sizeAvailable, which would require plumbing
> isAnimated up through DeferredImageDecoder and ImageSource; instead it happens
> inside PNGImageReader, when it reports the size, so no API changes are
> necessary.)

A downside to this approach (i.e. logging from inside the decoder/reader) is that if the same image is decoded with different ImageDecoders we will double count. This was previously true for some other metrics (e.g. this one: crrev.com/1984573004), but since the counts are relative it should not be a problem. That said, a proposed fix for  issue 706613  (not respecting a canvas's color space - relevant here particularly for the case where the same image is drawn to two different color spaces as in https://bugs.chromium.org/p/chromium/issues/detail?id=706613#c5) is to cache ImageDecoders in the ImageDecodingStore by their AlphaOption (i.e. one ImageDecoder may be used when no conversion is necessary, using premultiplied, and another may be used with unpremultiplied so we can do the conversion afterwards). When this happens (I suspect this would not be a common case?), we would count the image a total of three times.

> I think it might be interesting to track relative use of animated GIF
> versus animated PNG versus animated WEBP

https://codereview.chromium.org/2800343004 does exactly that.

In both cases I count broken APNGs as static, since that's how we interpret them, but maybe they should be counted as animated, since that was (presumably) the intention of the author?

Similarly, if an image takes advantage of APNG but only has one frame, I count that as animated, but maybe that should count as static? Or maybe it should even be ignored because that's generally used for verifying APNG support? (Or maybe it's a rare enough case in actual use that it does not matter?) Similarly, libwebp has a flag for animation, so in theory I could imagine that flag being set even though there is only one frame, but I haven't dug deeper to know whether those can be out of sync in practice.

> > I think it might be interesting to track relative use of animated GIF
> versus animated PNG versus animated WEBP

Agree.

Currently, in the code for BitmapImage::isSizeAvailable(), when we first get the size of the image, we record a stat there for all image types using:

if (size_available_ && HasVisibleImageSize(size())) {
  BitmapImageMetrics::CountDecodedImageType(source_.FilenameExtension());
  ...
}

ie., we read the FilenameExtension() from the ImageSource, hence the ImageDecoder, which is "png" for PNG, "gif" for GIF, "webp" for WEBP, and so on.

I imagined that if the image was an APNG, that could detected using the file extension too, and record a stat, with something like this ...

if (size_available_ && HasVisibleImageSize(size())) {
  auto type = source_.FilenameExtension();
  if (type == "png" && source_.isAnimated())
    BitmapImageMetrics::CountDecodedImageType("apng");
  else
    BitmapImageMetrics::CountDecodedImageType(type);
  ...
}

and update the XML histogram to understand the new string "apng", of course.  How does that sound?

> and update the XML histogram to understand the new string "apng", of course. 
> How does that sound?

It doesn't give us the comparison that I think might be the most interesting here: how often is APNG used relative to animated GIF and animated WEBP.

In addition, if anyone is looking at how the metrics change over time, we'll suddenly report less PNGs, which will have moved into the APNG bucket.

Lastly, isAnimated() will require adding APIs to a few classes (ImageDecoder, DeferredImageDecoder, and ImageSource). Not only do I dislike the idea of adding all these APIs that are only used by one piece of code (which itself is for tracking, not for functionality), but it seems like a confusing extra API, for the following reasons:

- GIF won't know until a second frame is seen, so its return value could change (FWIW, this is also true of ImageDecoder::RepetitionCount())
- an APNG can have only one frame. Do we count that as isAnimated? If we want to know "how often is APNG used?", it seems like the answer is yes, but if we want to know "how often is APNG used for animation?" maybe the answer is no?
- BitmapImage already has code to decide whether an image is animated based on the repetition count (maybe we should go with that instead of adding new APIs?)

> It doesn't give us the comparison that I think might be the most interesting here: how often is APNG used relative to animated GIF and animated WEBP.

That'd be ideal, but we are far from that place yet.

> In addition, if anyone is looking at how the metrics change over time, we'll suddenly report less PNGs, which will have moved into the APNG bucket.

Dunno why we'd worry about this.  All PNG are in a single bucket now.  We'd take that bucket and split into PNG and APNG.  Total number of "PNG" would be their sum, no?  Perhaps I'm missing something.

> Lastly, isAnimated() will require adding APIs to a few classes (ImageDecoder, DeferredImageDecoder, and ImageSource). Not only do I dislike the idea of adding all these APIs that are only used by one piece of code (which itself is for tracking, not for functionality), but it seems like a confusing extra API, for the following reasons:

isAnimated() was just an example name.  We'd pick something meaningful :)

Regarding plumbing: I think that since metrics are recorded in BitmapImage at this time, we have to plumb _something_ into the classes you mentioned (because they don't currently provide the needed information).

One idea might be to change FilenameExtension() to take a default bool argument:

  String ImageDecoder::FilenameExtension(bool forMetrics = false);

PNG example: it'd return "png" for the normal case, but FilenameExtension(true) would be used in BitmapImage when taking metrics.  In that case, it would return "apng" if the image header indicated it's an animated image.

> - GIF won't know until a second frame is seen, so its return value could change (FWIW, this is also true of ImageDecoder::RepetitionCount())

GIF is all in the same bucket now.  And the fact that we don't know if it is animated until it's decoded frame 2 is a pain.

> - an APNG can have only one frame. Do we count that as isAnimated? If we want to know "how often is APNG used?", it seems like the answer is yes, but if we want to know "how often is APNG used for animation?" maybe the answer is no?

I think we'd want to know how often apng used for now.  How often it's used for actual animation might be much harder to do reliably.

> - BitmapImage already has code to decide whether an image is animated based on the repetition count (maybe we should go with that instead of adding new APIs?)

Interesting.  Is it called for all images types (we could maybe record image metrics there if so)?   When it is called, is the repetition count known, or will it update to the true value later?

> > - BitmapImage already has code to decide whether an image is animated based on the repetition count (maybe we should go with that instead of adding new APIs?)

> Interesting.  Is it called for all images types (we could maybe record image metrics there if so)?   When it is called, is the repetition count known, or will it update to the true value later?

I was thinking BitmapImage again.

I think your proposal meant to call the decoder's->RepetitionCount() when we are gathering the metrics from within BitmapImage::isSizeAvailable() ?

Looking at the PNGImageReader [1], I see this:

  if (!is_animated_ || 1 == reported_frame_count_)
    decoder_->SetRepetitionCount(kCAnimationNone);

just before we SetSize() on the image.  We could indeed use that to work out if the PNG is animated when taking the metric.

> (maybe we should go with that instead of adding new APIs?)

Looks to me like that'd work fine.  

The APNG metric would mean: an APNG image with multiple frames, if I'm reading correctly?  Would something similar work for WEBP?

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp?type=cs&q=PNGImageReader+package:%5Echromium$&l=537



  

>> It doesn't give us the comparison that I think might be the most interesting
>> here: how often is APNG used relative to animated GIF and animated WEBP.
>
> That'd be ideal, but we are far from that place yet.

What are we missing? What is wrong with https://codereview.chromium.org/2800343004/ ? It double counts, but since they all double count, and we really only care about relative counts, that doesn't seem like a problem to me.

Is it that some images may be counted only once if their sizes get decoded for positioning, but the image is never displayed? (Is that something that often happens?)

The other issue I mentioned, where the same image is decoded in two different color spaces, so it gets triple counted, is rare, I would guess.

>> In addition, if anyone is looking at how the metrics change over time, we'll
>> suddenly report less PNGs, which will have moved into the APNG bucket.
>
> Dunno why we'd worry about this.  All PNG are in a single bucket now.  We'd
> take that bucket and split into PNG and APNG.  Total number of "PNG" would
> be their sum, no?  Perhaps I'm missing something.

You didn't answer my question about the goal, which I think would be helpful here. You are correct, their sum would be the total number of PNG, but that won't be displayed on the graph, so if someone is trying to see how PNG compares to other types, they have to know to combine the two bars. Furthermore, if someone is looking at a timeline view (when I was learning how to make a histogram I thought I saw something that implied there is a timeline view, although I haven't yet re-found it), the naive viewer might think that the number of PNGs suddenly dropped when it just got split. (This assumes that Chromium browsers are being served a substantial number of APNGs today; we don't know that to be true - if it's false, the timeline view won't appear to have an effect on the number of PNGs.)

Regardless, it seems weird to split up PNG without splitting up GIF and WEBP.

> I think we'd want to know how often apng used for now.  How often it's used
> for actual animation might be much harder to do reliably.

This is pretty straightfoward, as I think your next comment (comment #13) acknowledges.

>> - BitmapImage already has code to decide whether an image is animated based
>> on the repetition count (maybe we should go with that instead of adding new APIs?)
>
> Interesting.  Is it called for all images types (we could maybe record image
> metrics there if so)?   When it is called, is the repetition count known, or
> will it update to the true value later?

See BitmapImage::RepetitionCount. Yes, it is used for all image types. It assumes that the repetition count may change if the data is not complete.

> I think your proposal meant to call the decoder's->RepetitionCount()
> when we are gathering the metrics from within BitmapImage::isSizeAvailable() ?

No, if we were to use the RepetitionCount, I was thinking we should log inside BitmapImage::RepetitionCount, once we are certain about the repetition count. There is a pitfall though - if we don't fully receive the image, we may not log anything. We might be able to fix this (in some cases), but I think it would have to rely on internal knowledge of how the decoders work, so maybe this is not the way to go.

> Looking at the PNGImageReader [1], I see this:
>
>   if (!is_animated_ || 1 == reported_frame_count_)
>     decoder_->SetRepetitionCount(kCAnimationNone);
>
> just before we SetSize() on the image.  We could indeed use that to work
> out if the PNG is animated when taking the metric.

Agreed. Would not work for GIF though.

> The APNG metric would mean: an APNG image with multiple frames, if I'm
> reading correctly? 

Yes, although I'd rather pick a metric and implement based on the metric.

> Would something similar work for WEBP?

Yes. WEBP knows there are multiple frames when the size is retrieved.