Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1396323007:
API to support native scaling by image-generator (Closed)
DescriptionAPI to support native scaling by image-generator
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/7850eb2f357c215b2e2c50bf16d6c6df38c7967f
Patch Set 1 #
Total comments: 25
Patch Set 2 : #Patch Set 3 : update comment about inexact scaling support #Patch Set 4 : experiment w/ alternate comments and alternate signature #
Total comments: 10
Patch Set 5 : remove alternate versions #
Total comments: 6
Patch Set 6 : #Patch Set 7 : now with gm #
Total comments: 8
Patch Set 8 : remove min, fix warning #
Total comments: 7
Patch Set 9 : mark test image as opaque, so jpeg has a chance of encoding it #
Messages
Total messages: 41 (9 generated)
reed@google.com changed reviewers: + fmalita@chromium.org, msarett@google.com, scroggo@google.com
just reviewing API contract for now
This looks good. I think it is a good match with what we know we can do well right now. I'm in favor of extending this to use scanlines at some point as well. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:159: * the two nearest supported dimensions. If scaling is not supported, or scale is invalid Would we want to return one supportedSize that is smaller than the requested scale and one supportedSize that is larger than the requested scale, not necessarily the "two nearest"? Maybe this is a problem if the generator cannot scale small enough to be smaller than the requested scale? (it should always be able to scale larger) Would it useful to the client to guarantee that supportedSizes[0] is the attempt to scale smaller and supportedSizes[1] is the attempt to scale larger (or something like that)? https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:160: * this will return false, and supportedSizes[] will be undefined. Can we define an "invalid scale" as <= 0.0f or > 1.0f? https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:166: * If scaledSubset is not null, it specifies the subset of the scaled pixels that should nit: scaledClip https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:168: * valid if it is entire contained inside the scaled dimensions... nit: entirely https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:173: * If the requested scaledInfo is not supported by the generator, it it encounters an error, nit: it it https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, I have one concern about this that I thought of after our meeting. If we are asking for a scaledClip, the dimensions of pixels will not match the dimensions of scaledInfo, instead they will match the dimensions of scaledClip. I think this could be a little confusing. We should at least make this clear in the comments.
https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:174: * or the scaledClip is invalid, this returns false and the contents of pixels is undefined. In the case of an incomplete image, will this return true or false? Or would it be useful to return an SkCodec::Result? Or the number of lines decoded successfully (I don't think we provide this right now)?
It seems this new method does not support color tables? It seems like the new method is only different from getPixels in that it has a clip (getPixels is documented to reflect that non-matching info dimensions means a scale, although it also states that it can return kInvalidScale, which needs to be updated...), but this is not reflected in the name. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/13 21:43:23, msarett wrote: > I have one concern about this that I thought of after our meeting. > > If we are asking for a scaledClip, the dimensions of pixels will not match the > dimensions of scaledInfo, instead they will match the dimensions of scaledClip. > > I think this could be a little confusing. We should at least make this clear in > the comments. Agreed. I thought the conclusion we came to was that the generator could choose to only fill scaledClip, but the client would need to allocate all of scaledInfo.
https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:108: bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, I think this existing method (getPixels - based on documentation only) fits the generateScalledPixels use case already: " * A size that does not match getInfo() implies a request * to scale. If the generator cannot perform this scale, * it will return kInvalidScale. ... bool getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, SkPMColor ctable[], int* ctableCount); https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:159: * the two nearest supported dimensions. If scaling is not supported, or scale is invalid On 2015/10/13 21:43:22, msarett wrote: > Would we want to return one supportedSize that is smaller than the requested > scale and one supportedSize that is larger than the requested scale, not > necessarily the "two nearest"? Another option would be to always return equal or nearest larger dimension. Not sure what would be use case of nearest smaller dimension - is it when space for pixels is pre allocated and there is a wish to keep it within the byte count? Or, is it for low memory situation where it is OK to do upscaling while rendering? https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:162: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); since getPixels could be already (implicitly though) used for getting downscaled image pixels, maybe it makes sense to extend this method to ask if in general SkImageInfo can be supported: e.g. There is no API to check if it can be decoded to YUV, just getYUVPlanes: Inside Skia then, GrTexture* SkImageCacherator::lockAsTexture needs allocate planes etc from cache before calling getYUVPlanes, and that one could fail (e.g. JPEGImageDecoder::canDecodeToYUV()) I'm planning to post a review for that case tomorrow - in order to support RGB565 (partially decoded JPEG frames are having alpha on undecoded tiles) and RGB565 is only supported once the full data is available and can be decoded.
> I'm planning to post a review for that case tomorrow - in order to support > RGB565 (partially decoded JPEG frames are having alpha on undecoded tiles) and > RGB565 is only supported once the full data is available and can be decoded. I have extended this approach with support for decoding to different formats in new proposal - review request here https://codereview.chromium.org/1404923004 Thanks.
I plan to continue investigating this CL. I'm ok if you want to also explore a separate path, but realize that I think perhaps that duplication may not be efficient. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:159: * the two nearest supported dimensions. If scaling is not supported, or scale is invalid On 2015/10/13 21:43:22, msarett wrote: > Would we want to return one supportedSize that is smaller than the requested > scale and one supportedSize that is larger than the requested scale, not > necessarily the "two nearest"? Maybe this is a problem if the generator cannot > scale small enough to be smaller than the requested scale? (it should always be > able to scale larger) > > Would it useful to the client to guarantee that supportedSizes[0] is the attempt > to scale smaller and supportedSizes[1] is the attempt to scale larger (or > something like that)? Yea, I like the idea of having the results be sorted (first one is <= second one), and if possible, the first one is <= float_scale <= second one. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:160: * this will return false, and supportedSizes[] will be undefined. On 2015/10/13 21:43:22, msarett wrote: > Can we define an "invalid scale" as <= 0.0f or > 1.0f? Done.
On 2015/10/15 18:21:28, reed1 wrote: > I plan to continue investigating this CL. I'm ok if you want to also explore a > separate path, but realize that I think perhaps that duplication may not be > efficient. Not a problem (not having a concern around me wasting time - just an API method where I copied and extended your approach). Please just reconsider combining with decoding to other colorTypes, as described here: https://codereview.chromium.org/1404923004 and push. I'm working on RGB565 support in chromium and downscalling implementation no problem to move from one API glue to another, whatever is the final version. Thanks
gotcha
aleksandar.stojiljkovic@intel.com changed reviewers: + aleksandar.stojiljkovic@intel.com
https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, Related to 1412423008 and 1403393004 (downsample & RGB565 support during decode), I think it is better to move the control on allocation to SkImageGenerator implementation. That way, SkImageGenerator would't have to pre-allocate memory for pixel data, and make 2 calls: 1) what can you scale 2) give me scaled. Instead, seems better that through this API, allocator is supplied (discardable memory alloc) and scale and some hint about prefered output type - then generator decides what is the output format, allocates and selects appropriate config for the decoded bitmap (alpha, rgb565, indexed palette). For pngs, after completing partial decoding runs, and establishing that there is no alpha, generator could choose to drop previous 8888 allocation and replace by 565. For animated gifs, if it chooses to decode to index8, generator could choose to instantiate new one (with different palette) if current one has palette changed.
https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:159: * the two nearest supported dimensions. If scaling is not supported, or scale is invalid On 2015/10/13 21:43:22, msarett wrote: > Would we want to return one supportedSize that is smaller than the requested > scale and one supportedSize that is larger than the requested scale, not > necessarily the "two nearest"? Maybe this is a problem if the generator cannot > scale small enough to be smaller than the requested scale? (it should always be > able to scale larger) > > Would it useful to the client to guarantee that supportedSizes[0] is the attempt > to scale smaller and supportedSizes[1] is the attempt to scale larger (or > something like that)? Your last suggestion makes sense to me. I'll update the comment. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:162: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); On 2015/10/14 15:32:35, aleksandar.stojiljkovic wrote: > since getPixels could be already (implicitly though) used for getting downscaled > image pixels, maybe it makes sense to extend this method to ask if in general > SkImageInfo can be supported: > e.g. > There is no API to check if it can be decoded to YUV, just getYUVPlanes: > Inside Skia then, GrTexture* SkImageCacherator::lockAsTexture needs allocate > planes etc from cache before calling getYUVPlanes, and that one could fail (e.g. > JPEGImageDecoder::canDecodeToYUV()) > > I'm planning to post a review for that case tomorrow - in order to support > RGB565 (partially decoded JPEG frames are having alpha on undecoded tiles) and > RGB565 is only supported once the full data is available and can be decoded. Perhaps we do want more query methods, but this one is different than asking if a requested imageinfo would work, since it doesn't propose a width/height, but rather is asking for possible width/height pairs. If we want other types of queries (e.g. YUV, etc.) I suggest we add those separately as needed. We can always consolidate methods afterwards if we see the opportunity. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:166: * If scaledSubset is not null, it specifies the subset of the scaled pixels that should On 2015/10/13 21:43:23, msarett wrote: > nit: scaledClip Done. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:168: * valid if it is entire contained inside the scaled dimensions... On 2015/10/13 21:43:23, msarett wrote: > nit: entirely Done. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:173: * If the requested scaledInfo is not supported by the generator, it it encounters an error, On 2015/10/13 21:43:23, msarett wrote: > nit: it it Done. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:174: * or the scaledClip is invalid, this returns false and the contents of pixels is undefined. On 2015/10/13 21:50:31, msarett wrote: > In the case of an incomplete image, will this return true or false? Or would it > be useful to return an SkCodec::Result? Or the number of lines decoded > successfully (I don't think we provide this right now)? Good question. I *think* at the image-generator level, this is a simpler issue. Generators are one-shots, in that they are required to always return the same answer each time. Thus, if the provided data is incomplete (partial), that specific generator will always return the same number of decoded pixels. Is it important for the generator to report this? Not sure. It seems like the answer could be that all we need to handle is what-to-stick in the un-decoded pixels... transparent or some special color. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/14 14:11:28, scroggo wrote: > On 2015/10/13 21:43:23, msarett wrote: > > I have one concern about this that I thought of after our meeting. > > > > If we are asking for a scaledClip, the dimensions of pixels will not match the > > dimensions of scaledInfo, instead they will match the dimensions of > scaledClip. > > > > I think this could be a little confusing. We should at least make this clear > in > > the comments. > > Agreed. I thought the conclusion we came to was that the generator could choose > to only fill scaledClip, but the client would need to allocate all of > scaledInfo. Ah, I had a different idea. I think one of the key values of scaling/clipping is so the client can save RAM (and hopefully go faster because of that). Thus I think the scaledClip represents exactly how many pixels the client has allocated. That said, it is confusing, since the caller is sending in a width/height (info) and then a different width/height associated with the clip. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/27 13:42:25, aleksandar.stojiljkovic wrote: > Related to 1412423008 and 1403393004 (downsample & RGB565 support during > decode), I think it is better to move the control on allocation to > SkImageGenerator implementation. > That way, SkImageGenerator would't have to pre-allocate memory for pixel data, > and make 2 calls: 1) what can you scale 2) give me scaled. > Instead, seems better that through this API, allocator is supplied (discardable > memory alloc) and scale and some hint about prefered output type - then > generator decides what is the output format, allocates and selects appropriate > config for the decoded bitmap (alpha, rgb565, indexed palette). > For pngs, after completing partial decoding runs, and establishing that there is > no alpha, generator could choose to drop previous 8888 allocation and replace by > 565. > For animated gifs, if it chooses to decode to index8, generator could choose to > instantiate new one (with different palette) if current one has palette changed. The older android decoders (SkImageDecoder) follow this pattern nearly exactly. It has also proven to have problems of control and clear-expectations. ImageGenerator exists partly to precisely change the pattern to one where the caller is in more control of what they want and where they want it. For example, the new pattern makes it clear how a caller might re-use a previous allocation (or part of a previous allocation) without having to trick-out the "allocator" ahead of time.
https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:159: * the two nearest supported dimensions. If scaling is not supported, or scale is invalid On 2015/10/28 18:01:51, reed1 wrote: > On 2015/10/13 21:43:22, msarett wrote: > > Would we want to return one supportedSize that is smaller than the requested > > scale and one supportedSize that is larger than the requested scale, not > > necessarily the "two nearest"? Maybe this is a problem if the generator > cannot > > scale small enough to be smaller than the requested scale? (it should always > be > > able to scale larger) > > > > Would it useful to the client to guarantee that supportedSizes[0] is the > attempt > > to scale smaller and supportedSizes[1] is the attempt to scale larger (or > > something like that)? > > Your last suggestion makes sense to me. I'll update the comment. I'm not sure I see an update to the comment? https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:174: * or the scaledClip is invalid, this returns false and the contents of pixels is undefined. On 2015/10/28 18:01:51, reed1 wrote: > On 2015/10/13 21:50:31, msarett wrote: > > In the case of an incomplete image, will this return true or false? Or would > it > > be useful to return an SkCodec::Result? Or the number of lines decoded > > successfully (I don't think we provide this right now)? > > Good question. I *think* at the image-generator level, this is a simpler issue. > Generators are one-shots, in that they are required to always return the same > answer each time. Thus, if the provided data is incomplete (partial), that > specific generator will always return the same number of decoded pixels. > > Is it important for the generator to report this? Not sure. It seems like the > answer could be that all we need to handle is what-to-stick in the un-decoded > pixels... transparent or some special color. From what I've seen, *most* of the incomplete images that clients expect us to decode are almost complete, and it would be best to fill in the remaining pixels and return true. So I think this is probably fine - I'm not sure that it's important for the generator to report this. Leon and I have discussed failing on images that are missing lots of pixels, as opposed to missing just a few, but we haven't implemented this yet. So as of now, this API may return true and return a mostly blank image. I think this is fine for bad images. I think you're right - the issue that needs to be handled is what-to-stick in the un-decoded pixels. Chrome leaves them transparent. We don't always have this option because we have more color types (565, Index8, Gray, etc.). Right now we decide what to fill with for the client. As you've mentioned, I think this is a flaw in the Codec API. I intend to fix this so the client can provide a fill color if they desire. https://codereview.chromium.org/1396323007/diff/1/include/core/SkImageGenerat... include/core/SkImageGenerator.h:176: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/28 18:01:51, reed1 wrote: > On 2015/10/14 14:11:28, scroggo wrote: > > On 2015/10/13 21:43:23, msarett wrote: > > > I have one concern about this that I thought of after our meeting. > > > > > > If we are asking for a scaledClip, the dimensions of pixels will not match > the > > > dimensions of scaledInfo, instead they will match the dimensions of > > scaledClip. > > > > > > I think this could be a little confusing. We should at least make this > clear > > in > > > the comments. > > > > Agreed. I thought the conclusion we came to was that the generator could > choose > > to only fill scaledClip, but the client would need to allocate all of > > scaledInfo. > > Ah, I had a different idea. > > I think one of the key values of scaling/clipping is so the client can save RAM > (and hopefully go faster because of that). Thus I think the scaledClip > represents exactly how many pixels the client has allocated. > > That said, it is confusing, since the caller is sending in a width/height (info) > and then a different width/height associated with the clip. I think whatever decision is made here should and probably will trickle into the SkCodec API as well.
https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:170: * in sukpportedSizes[1] nit: *supported https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:177: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); I prefer these comments. https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:189: * support scaling to those values (e.g. JPEG typically can only scale by powers of 2). JPEG gives us all the eighths. Ex: 1/8, 1/4, 3/8, 1/2, 5/8, 3/4, 7/8. But powers of 2 tend to be a bit more efficient. https://docs.google.com/spreadsheets/d/1V_-6seZXa0h2flox1bfOO3WQQtplrk5F7I26d... https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:210: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, I'm indifferent on this, but I lean towards this API.
https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:210: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, if SkImageInfo can be supplied here, i.e. 565 or Index8, could there also be a query for getting supported colorTypes?
https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:170: * in sukpportedSizes[1] On 2015/10/29 19:50:24, msarett wrote: > nit: *supported Done. https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:177: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); On 2015/10/29 19:50:24, msarett wrote: > I prefer these comments. Done. https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:189: * support scaling to those values (e.g. JPEG typically can only scale by powers of 2). On 2015/10/29 19:50:24, msarett wrote: > JPEG gives us all the eighths. Ex: 1/8, 1/4, 3/8, 1/2, 5/8, 3/4, 7/8. > > But powers of 2 tend to be a bit more efficient. > https://docs.google.com/spreadsheets/d/1V_-6seZXa0h2flox1bfOO3WQQtplrk5F7I26d... Gone https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:210: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/29 19:50:24, msarett wrote: > I'm indifferent on this, but I lean towards this API. Done. https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... include/core/SkImageGenerator.h:210: bool generateScaledPixels(const SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, On 2015/10/29 20:39:35, aleksandar.stojiljkovic wrote: > if SkImageInfo can be supplied here, i.e. 565 or Index8, could there also be a > query for getting supported colorTypes? 1. 565 Decoding (generating) into 565 is pretty harsh, since there is (typically) a loss of quality, and questions about dithering (unless we're not going to dither either). What is your interest in getting scaled output into 565? 2. Index8 Supporting Index8 (it seems to me) is binary -- either the original colortype is Index8, in which case we can subset or scale (sans any filtering/dithering) into it as well. If the original is *not* Index8, then I propose that we never expect the generator to convert into Index8.
On 2015/11/02 20:52:59, reed1 wrote: > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > File include/core/SkImageGenerator.h (right): > > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > include/core/SkImageGenerator.h:170: * in sukpportedSizes[1] > On 2015/10/29 19:50:24, msarett wrote: > > nit: *supported > > Done. > > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > include/core/SkImageGenerator.h:177: bool computeScaledDimensions(SkScalar > scale, SkISize supportedSizes[2]); > On 2015/10/29 19:50:24, msarett wrote: > > I prefer these comments. > > Done. > > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > include/core/SkImageGenerator.h:189: * support scaling to those values (e.g. > JPEG typically can only scale by powers of 2). > On 2015/10/29 19:50:24, msarett wrote: > > JPEG gives us all the eighths. Ex: 1/8, 1/4, 3/8, 1/2, 5/8, 3/4, 7/8. > > > > But powers of 2 tend to be a bit more efficient. > > > https://docs.google.com/spreadsheets/d/1V_-6seZXa0h2flox1bfOO3WQQtplrk5F7I26d... > > Gone > > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > include/core/SkImageGenerator.h:210: bool generateScaledPixels(const > SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, > On 2015/10/29 19:50:24, msarett wrote: > > I'm indifferent on this, but I lean towards this API. > > Done. > > https://codereview.chromium.org/1396323007/diff/60001/include/core/SkImageGen... > include/core/SkImageGenerator.h:210: bool generateScaledPixels(const > SkImageInfo& scaledInfo, void* pixels, size_t rowBytes, > On 2015/10/29 20:39:35, aleksandar.stojiljkovic wrote: > > if SkImageInfo can be supplied here, i.e. 565 or Index8, could there also be a > > query for getting supported colorTypes? > > 1. 565 > > Decoding (generating) into 565 is pretty harsh, since there is (typically) a > loss of quality, and questions about dithering (unless we're not going to dither > either). What is your interest in getting scaled output into 565? > > 2. Index8 > > Supporting Index8 (it seems to me) is binary -- either the original colortype is > Index8, in which case we can subset or scale (sans any filtering/dithering) into > it as well. If the original is *not* Index8, then I propose that we never expect > the generator to convert into Index8. > 1. 565 What is your interest in getting scaled output into 565? It is important for Android and low end devices in general (as the resolution gets higher, surface is often RGB565, especially with lower end CPU/GPU with higher screen resolution), so why not use 565 bitmaps where it allows and so that it can be controlled from Chromium. Example here: ccrev.com/1412423008 Handling large jpegs in CPU raster (downsample to display res and 565). In linked bugs, for 3 picture compare views within viewport, SkResourceCache requirement is: 130MB (RGBA_8888), 33MB(scaled down to 3/8 or 4/8 RGBA_8888), 16MB(scaled down to 3/8 or 4/8 RGB_565). Diether: Except for CMYK (and maybe RGB), Chromium Jpeg decoder is doing diethering (we don't need to do it). For png I plan to do it as an option (if it is really transparent). Agree, because of affinity loss, 565 might not be active by default in Chromium on Android, but important to have as an option; when detected that it is needed. e.g. it can be detected by monitoring the state in discardable memory - how many SkOneShotDiscardablePixelRef lost their unpinned ashmem pixels. It already looks like I'm overselling the need for this (and moving to subjective argumentation). It is just enough to run that page with compare views with downsampling and RGB_565, which makes re-decoding very rare (there is still often cache evicting happening with downsampled RGBA_8888) to notice how better the experience is. 2. OK, safe to assume that one frame from animated gif is Index8 before doing the decoding.
https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:172: * requested scale, then that 1 native size will returned in both [0] and [1]. will be* returned https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:177: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); As stated in person, I'd prefer something other than an array here. Something like one of the following: bool computeScaledDimensions(SkScalar scale, SkISize* smallerNativeSize, SkISize* largerNativeSize); or struct sizes { SkISize smaller; SkISize larger; }; bool computeScaledDimensions(SkScalar scale, sizes* supportedSizes); I find these better for a couple of reasons: - the variables are named (rather than remembering the difference between supportedSizes[0] and supportedSizes[1]) - the compiler will enforce that you pass the correct number of things to computeScaledDimensions https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:178: Should computeScaledDimensions be const?
https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:172: * requested scale, then that 1 native size will returned in both [0] and [1]. On 2015/11/03 16:59:19, scroggo wrote: > will be* returned Done. https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:177: bool computeScaledDimensions(SkScalar scale, SkISize supportedSizes[2]); On 2015/11/03 16:59:19, scroggo wrote: > As stated in person, I'd prefer something other than an array here. Something > like one of the following: > > bool computeScaledDimensions(SkScalar scale, SkISize* smallerNativeSize, > SkISize* largerNativeSize); > > or > > struct sizes { > SkISize smaller; > SkISize larger; > }; > > bool computeScaledDimensions(SkScalar scale, sizes* supportedSizes); > > I find these better for a couple of reasons: > - the variables are named (rather than remembering the difference between > supportedSizes[0] and supportedSizes[1]) > - the compiler will enforce that you pass the correct number of things to > computeScaledDimensions Done. https://codereview.chromium.org/1396323007/diff/80001/include/core/SkImageGen... include/core/SkImageGenerator.h:178: On 2015/11/03 16:59:19, scroggo wrote: > Should computeScaledDimensions be const? None of the other methods are, though *all* are meant to always return the same value, even if called multiple times.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1396323007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1396323007/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
This looks good to me. I think that supporting scaling (no subsets yet) will be straightforward for SkCodec. I'll try to create an SkCodecImageGenerator to see how that looks with this API. https://codereview.chromium.org/1396323007/diff/120001/include/core/SkImageGe... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/120001/include/core/SkImageGe... include/core/SkImageGenerator.h:168: bool computeScaledDimensions(SkScalar scale, SupportedSizes*); I remember that we wanted fSizes[0] to be smaller if possible and fSizes[1] to be larger if possible? It's probably fine not to mention this in the comments since I don't think the initial implementations will behave this way anyway. https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... File src/core/SkPictureImageGenerator.cpp (right): https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:84: bool SkPictureImageGenerator::onComputeScaledDimensions(SkScalar scale, This can be implemented for SkCodec by: SkISize scaledSize = codec->getScaledDimensions(scale); if (scaledSize == codec->getInfo().dimensions) { return false; } // You could also check that scaledSize isn't zero (since we return zero for // invalid scales), but you already check for invalid scales, so I think this // is enough. sizes->fSizes[0] = scaledSize; sizes->fSizes[1] = scaledSize; SkCodec isn't ready to provide a smaller and a larger size option. https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:88: const int sw = SkMin32(SkScalarRoundToInt(scale * w), w); Won't SkMin32 be unnecessary, since invalid scales will be caught in computeScaledDimensions()? https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:98: bool SkPictureImageGenerator::onGenerateScaledPixels(const SkISize& scaledSize, This can be implemented for SkCodec by: codec->getPixels(scaledPixels.info(), <memory>, scaledPixels.rowBytes()); (or using a more complicated version if you want kIndex8) But subsets aren't ready yet. scaledSize will need to match scaledPixels.size() and scaledOrigin will need to be (0, 0).
https://codereview.chromium.org/1396323007/diff/120001/include/core/SkImageGe... File include/core/SkImageGenerator.h (right): https://codereview.chromium.org/1396323007/diff/120001/include/core/SkImageGe... include/core/SkImageGenerator.h:168: bool computeScaledDimensions(SkScalar scale, SupportedSizes*); On 2015/11/30 21:24:26, msarett wrote: > I remember that we wanted fSizes[0] to be smaller if possible and fSizes[1] to > be larger if possible? > > It's probably fine not to mention this in the comments since I don't think the > initial implementations will behave this way anyway. I'm not sure what "smaller" means anymore, since there are two values. I removed that language on purpose, though happy to rethink about it. https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... File src/core/SkPictureImageGenerator.cpp (right): https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:84: bool SkPictureImageGenerator::onComputeScaledDimensions(SkScalar scale, On 2015/11/30 21:24:26, msarett wrote: > This can be implemented for SkCodec by: > > SkISize scaledSize = codec->getScaledDimensions(scale); > if (scaledSize == codec->getInfo().dimensions) { > return false; > } > // You could also check that scaledSize isn't zero (since we return zero for > // invalid scales), but you already check for invalid scales, so I think this > // is enough. > > sizes->fSizes[0] = scaledSize; > sizes->fSizes[1] = scaledSize; > > SkCodec isn't ready to provide a smaller and a larger size option. Ack. That said, I think returning 2 sizes will be sort of valuable for JPEG, which might want to return POW2 answers. https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:88: const int sw = SkMin32(SkScalarRoundToInt(scale * w), w); On 2015/11/30 21:24:26, msarett wrote: > Won't SkMin32 be unnecessary, since invalid scales will be caught in > computeScaledDimensions()? Good catch. https://codereview.chromium.org/1396323007/diff/120001/src/core/SkPictureImag... src/core/SkPictureImageGenerator.cpp:98: bool SkPictureImageGenerator::onGenerateScaledPixels(const SkISize& scaledSize, On 2015/11/30 21:24:26, msarett wrote: > This can be implemented for SkCodec by: > > codec->getPixels(scaledPixels.info(), <memory>, scaledPixels.rowBytes()); > (or using a more complicated version if you want kIndex8) > > But subsets aren't ready yet. scaledSize will need to match scaledPixels.size() > and scaledOrigin will need to be (0, 0). Acknowledged.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1396323007/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1396323007/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode353 gm/image.cpp:353: static SkImageGenerator* gen_jpg(const SkImageInfo& info, GrContext*) { I'm not sure this is working right. JPEGs must be opaque. https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode404 gm/image.cpp:404: const SkImageInfo info = SkImageInfo::MakeN32Premul(100, 100); SkCodec fails if we attempt to decode a jpeg to kPremul_SkAlphaType.
https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode353 gm/image.cpp:353: static SkImageGenerator* gen_jpg(const SkImageInfo& info, GrContext*) { On 2015/11/30 23:00:32, msarett wrote: > I'm not sure this is working right. JPEGs must be opaque. You're right. The test will be changed to present an opaque image.
lgtm https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode377 gm/image.cpp:377: const SkImageInfo info = SkImageInfo::MakeN32Premul(sizes.fSizes[0].width(), Why not draw both sizes? https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode404 gm/image.cpp:404: const SkImageInfo info = SkImageInfo::MakeN32Premul(100, 100); On 2015/11/30 23:00:32, msarett wrote: > SkCodec fails if we attempt to decode a jpeg to kPremul_SkAlphaType. I've gone back and forth on whether that's the right behavior. An opaque image is legitimately premul or unpremul, so we *could* support it, and I thought I saw somewhere else where we support such a "conversion". IIRC, the motivation behind failing is that it forces the client to use opaque, which will allow us to draw faster.
https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode377 gm/image.cpp:377: const SkImageInfo info = SkImageInfo::MakeN32Premul(sizes.fSizes[0].width(), On 2015/12/02 15:04:44, scroggo wrote: > Why not draw both sizes? Could, but I thought the key here was to see the "best" answer, or at least that the generate returned "an" answer (as some of them won't). https://codereview.chromium.org/1396323007/diff/140001/gm/image.cpp#newcode404 gm/image.cpp:404: const SkImageInfo info = SkImageInfo::MakeN32Premul(100, 100); On 2015/11/30 23:00:32, msarett wrote: > SkCodec fails if we attempt to decode a jpeg to kPremul_SkAlphaType. I think encode+premul -> jpeg can fail (or not, your call) I think decode+jpeg -> premul should definitely succeed, knowing that the query on the code/geneartor will "claim" to be opaque
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1396323007/#ps160001 (title: "mark test image as opaque, so jpeg has a chance of encoding it")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1396323007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1396323007/160001
Message was sent while issue was closed.
Description was changed from ========== API to support native scaling by image-generator BUG=skia: ========== to ========== API to support native scaling by image-generator BUG=skia: Committed: https://skia.googlesource.com/skia/+/7850eb2f357c215b2e2c50bf16d6c6df38c7967f ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/7850eb2f357c215b2e2c50bf16d6c6df38c7967f |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
