Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1718273004:
Add an SkImageGeneratorCG (Closed)
DescriptionAdd an SkImageGeneratorCG
This will serve as a replacement for SkImageDecoder_CG.
BUG=skia:4914
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1718273004
Committed: https://skia.googlesource.com/skia/+/1897631ebe417631ea7a046a2eb0995ab9d04539
Patch Set 1 : #Patch Set 2 : Use CGCopyProperties to perform a deferred decode #
Total comments: 8
Patch Set 3 : Respond to comments #
Total comments: 12
Patch Set 4 : Response to comments #Patch Set 5 : Remove changes to CodecImGen, sorry about the rebase #Patch Set 6 : Disable RAW for image generators #Patch Set 7 : Blacklist questionable bmps on CG #
Total comments: 2
Patch Set 8 : Partial match on blacklist, Bug for RAW #Patch Set 9 : 80 chars #
Messages
Total messages: 51 (24 generated)
Description was changed from ========== Add an SkImageGeneratorCG This will serve as a replacement for SkImageDecoder_CG. BUG=skia:4914 ========== to ========== Add an SkImageGeneratorCG This will serve as a replacement for SkImageDecoder_CG. BUG=skia:4914 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + corey.lucier@gmail.com, reed@google.com, scroggo@google.com
I haven't hooked this in anywhere except for test code. Seems simple enough for the client to write their own version of SkImageGenerator_skia.cpp or to create a ImageGeneratorFromEncodedFactory.
does CG return a generator in the sense that it hasn't yet paid for the decode and allocation, or has CG already done that, in which case perhaps this guy returning an SkImage would be more honest/efficient?
On 2016/02/25 18:22:51, reed1 wrote: > does CG return a generator in the sense that it hasn't yet paid for the decode > and allocation, or has CG already done that, in which case perhaps this guy > returning an SkImage would be more honest/efficient? SkImageGeneratorCG::NewFromEncodedCG() pays for a decode and allocation. I would agree that returning an SkImage would be more honest/efficient. I'll rework this code into SkImage::NewFromEncoded().
Thank you for adding this. I assume the equivalent of SkImageDecoder_WIC will follow as well? (The Windows platform equivalent of SkImageDecoder_CG) ?
Yes that's correct. This is still a WIP, but we will finish adding alternative support for CG and WIC before deleting SkImageDecoder.
Patchset #2 (id:120001) has been deleted
PTAL We now get width, height, alpha properties from the CG image source, so we can defer the decode until the call to onGetPixels().
https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... File src/ports/SkImageGeneratorCG.cpp (right): https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:46: kCGImagePropertyHasAlpha)); Do we know that hasalpha means its not opaque? Do we have some test images to try to experimentally find out? https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:53: SkImageInfo info = SkImageInfo::Make(width, height, kN32_SkColorType, alphaType); It appears that we can query properties to discover other things too, like if the image has float components or a colorprofile. Perhaps we need to think about how those things get communicated from generators to the client (they way codec is evolving)... Also, do these properties tell us if the data is unpremul or premul?
https://codereview.chromium.org/1718273004/diff/140001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/1718273004/diff/140001/gyp/ports.gyp#newcode75 gyp/ports.gyp:75: '../src/ports/SkImageGeneratorCG.cpp', This file seems to have gotten messy. Sometimes we include an OS specific file here, sometimes we remove it later, and sometimes we instead only add it to the proper OS. I don't know what to recommend here, but I expected to only build this file on Mac/iOS, so the build guards wouldn't be needed in the cpp file. https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... File src/ports/SkImageGeneratorCG.cpp (right): https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:81: // kUnpremul, we must unpremultiply. I think this is the same as the old one, but this sort of goes against the idea of only doing what we can do efficiently, doesn't it?
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Thanks for your comments! I've dug a lot deeper and ultimately decided not to change much. It's nice to know that we can support kUnpremul, 64-bit colors, color spaces etc. But I'd like to keep this simple for now and add those features to SkCodec first. https://codereview.chromium.org/1718273004/diff/140001/gyp/ports.gyp File gyp/ports.gyp (right): https://codereview.chromium.org/1718273004/diff/140001/gyp/ports.gyp#newcode75 gyp/ports.gyp:75: '../src/ports/SkImageGeneratorCG.cpp', On 2016/03/02 13:08:14, scroggo wrote: > This file seems to have gotten messy. Sometimes we include an OS specific file > here, sometimes we remove it later, and sometimes we instead only add it to the > proper OS. I don't know what to recommend here, but I expected to only build > this file on Mac/iOS, so the build guards wouldn't be needed in the cpp file. Now we only build this file on Mac/iOS. Removed build guards from the file. https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... File src/ports/SkImageGeneratorCG.cpp (right): https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:46: kCGImagePropertyHasAlpha)); On 2016/03/02 01:45:42, reed1 wrote: > Do we know that hasalpha means its not opaque? Do we have some test images to > try to experimentally find out? I tested this experimentally. We can trust hasAlpha to tell us whether the image is opaque or not. https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:53: SkImageInfo info = SkImageInfo::Make(width, height, kN32_SkColorType, alphaType); On 2016/03/02 01:45:42, reed1 wrote: > It appears that we can query properties to discover other things too, like if > the image has float components or a colorprofile. Perhaps we need to think about > how those things get communicated from generators to the client (they way codec > is evolving)... > > Also, do these properties tell us if the data is unpremul or premul? Strangely, we can't query for unpremul/premul until after we've decoded the image. Still it's safe to assume that all images are encoded as unpremul. Unfortunately, I've decided to hold off on supporting unpremul... It requires a rewrite to the swizzler to support Apple output formats. I think communicating float information isn't helpful (all of my test images indicate that they have ints). We will want to take advantage of 16-bits per component ints in the future though. Also holding off on getting the color profile until we know what to do with it. https://codereview.chromium.org/1718273004/diff/140001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:81: // kUnpremul, we must unpremultiply. On 2016/03/02 13:08:14, scroggo wrote: > I think this is the same as the old one, but this sort of goes against the idea > of only doing what we can do efficiently, doesn't it? Removed this code.
https://codereview.chromium.org/1718273004/diff/200001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1718273004/diff/200001/dm/DM.cpp#newcode331 dm/DM.cpp:331: static void push_image_gen_src(Path path, ImageGenSrc::Mode mode, SkAlphaType alphaType, bool isGpu) { nit: 100 chars https://codereview.chromium.org/1718273004/diff/200001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1718273004/diff/200001/dm/DMSrcSink.h#newcode193 dm/DMSrcSink.h:193: kCG_Mode, // Use CG Should this instead be something like kPlatform mode? It seems like we'll have an enum for each platform, even though only one will run. (I guess we'll just have two - CG and WIC.) https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... File src/ports/SkImageGeneratorCG.cpp (right): https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:37: SkAutoTCallVProc<const void, CFRelease> autoImageSrc(imageSrc); Why do you treat this as a const void instead of a CGImageSourceRef? https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:52: CFNumberGetValue(widthRef, kCFNumberIntType, &width); It looks like these return whether or not they succeed (otherwise they would just return the value you want, probably). We should probably check the return value and exit early if they fail. https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:77: if (kN32_SkColorType != info.colorType()) { FIXME: Support other types? https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:101: // restricts the color and alpha types that we support. If we I'd be curious about the performance difference, too, since we optimized the swizzler with SIMD instructions.
https://codereview.chromium.org/1718273004/diff/200001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1718273004/diff/200001/dm/DM.cpp#newcode331 dm/DM.cpp:331: static void push_image_gen_src(Path path, ImageGenSrc::Mode mode, SkAlphaType alphaType, bool isGpu) { On 2016/03/07 13:04:30, scroggo wrote: > nit: 100 chars Done. https://codereview.chromium.org/1718273004/diff/200001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1718273004/diff/200001/dm/DMSrcSink.h#newcode193 dm/DMSrcSink.h:193: kCG_Mode, // Use CG On 2016/03/07 13:04:30, scroggo wrote: > Should this instead be something like kPlatform mode? It seems like we'll have > an enum for each platform, even though only one will run. (I guess we'll just > have two - CG and WIC.) kPlatform SGTM. Good idea! No reason to have separate modes for CG and WIC. https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... File src/ports/SkImageGeneratorCG.cpp (right): https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:37: SkAutoTCallVProc<const void, CFRelease> autoImageSrc(imageSrc); On 2016/03/07 13:04:30, scroggo wrote: > Why do you treat this as a const void instead of a CGImageSourceRef? CFRelease must be used to free the CGImageSrcRef, but CFRelease takes a const void*. So we must treat the imageSrc as a const void*. I'll add a comment. https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:52: CFNumberGetValue(widthRef, kCFNumberIntType, &width); On 2016/03/07 13:04:30, scroggo wrote: > It looks like these return whether or not they succeed (otherwise they would > just return the value you want, probably). We should probably check the return > value and exit early if they fail. Done. https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:77: if (kN32_SkColorType != info.colorType()) { On 2016/03/07 13:04:30, scroggo wrote: > FIXME: Support other types? Done. https://codereview.chromium.org/1718273004/diff/200001/src/ports/SkImageGener... src/ports/SkImageGeneratorCG.cpp:101: // restricts the color and alpha types that we support. If we On 2016/03/07 13:04:30, scroggo wrote: > I'd be curious about the performance difference, too, since we optimized the > swizzler with SIMD instructions. Me too :). Adding to the FIXME.
lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718273004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718273004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@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/1718273004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718273004/240001
The CQ bit was unchecked by msarett@google.com
Patchset #7 (id:280001) has been deleted
PTAL (1) Removed modifications from SkCodecImageGenerator. (2) Stopped testing RAW images on image generator tests. I don't think there is any reason to run these tests. On the other hand, I'm not sure why they were failing. (3) Blacklisted questionable bmps on CG.
On 2016/03/09 18:31:04, msarett wrote: > PTAL > > (1) Removed modifications from SkCodecImageGenerator. Is this just adding back in make_premul, so it's always premultiplied? > (2) Stopped testing RAW images on image generator tests. I don't think there is > any reason to run these tests. On the other hand, I'm not sure why they were > failing. It would be nice to know why they're failing. If it's just OOM (we've added more raw images to test), I think it's fine to blacklist. But it seems like we could have some other problem. > (3) Blacklisted questionable bmps on CG. What does CG do for these BMPs? If it just returns false, blacklisting makes sense. If it crashes or has some other problem, I guess we'll still have to blacklist (and maybe file a bug with Apple?). https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py#newc... tools/dm_flags.py:118: blacklist.extend('_ image gen_platform_opaque rgba32abf.bmp'.split(' ')) Doesn't the blacklist check for partial matches? e.g. could you cut these in half by just using "gen_platform" instead of "gen_platform_opaque" and "gen_platform_premul"?
On 2016/03/09 19:44:01, scroggo wrote: > On 2016/03/09 18:31:04, msarett wrote: > > PTAL > > > > (1) Removed modifications from SkCodecImageGenerator. > > Is this just adding back in make_premul, so it's always premultiplied? > Yes. Otherwise, I break a bunch of gms. I think there's an API rewrite needed here, so callers to SkImage::NewFromGenerator() can specify color/alpha types. For now, I just want to leave this as is. > > (2) Stopped testing RAW images on image generator tests. I don't think there > is > > any reason to run these tests. On the other hand, I'm not sure why they were > > failing. > > It would be nice to know why they're failing. If it's just OOM (we've added more > raw images to test), I think it's fine to blacklist. But it seems like we could > have some other problem. > Agreed, I'll actually run CG locally against our set of DNGs. If I can't figure it out, I'll file a bug. > > (3) Blacklisted questionable bmps on CG. > > What does CG do for these BMPs? If it just returns false, blacklisting makes > sense. If it crashes or has some other problem, I guess we'll still have to > blacklist (and maybe file a bug with Apple?). > They just fail. Blacklist seems like the right call. > https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py > File tools/dm_flags.py (right): > > https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py#newc... > tools/dm_flags.py:118: blacklist.extend('_ image gen_platform_opaque > rgba32abf.bmp'.split(' ')) > Doesn't the blacklist check for partial matches? e.g. could you cut these in > half by just using "gen_platform" instead of "gen_platform_opaque" and > "gen_platform_premul"? Will fix this.
On 2016/03/09 19:54:53, msarett wrote: > On 2016/03/09 19:44:01, scroggo wrote: > > On 2016/03/09 18:31:04, msarett wrote: > > > PTAL > > > > > > (1) Removed modifications from SkCodecImageGenerator. > > > > Is this just adding back in make_premul, so it's always premultiplied? > > > > Yes. Otherwise, I break a bunch of gms. I think there's an API rewrite needed > here, so callers to SkImage::NewFromGenerator() can specify color/alpha types. > > For now, I just want to leave this as is. > > > > (2) Stopped testing RAW images on image generator tests. I don't think > there > > is > > > any reason to run these tests. On the other hand, I'm not sure why they > were > > > failing. > > > > It would be nice to know why they're failing. If it's just OOM (we've added > more > > raw images to test), I think it's fine to blacklist. But it seems like we > could > > have some other problem. > > > > Agreed, I'll actually run CG locally against our set of DNGs. If I can't figure > it out, I'll file a bug. > > > > (3) Blacklisted questionable bmps on CG. > > > > What does CG do for these BMPs? If it just returns false, blacklisting makes > > sense. If it crashes or has some other problem, I guess we'll still have to > > blacklist (and maybe file a bug with Apple?). > > > > They just fail. Blacklist seems like the right call. And by fail, I mean return false. > > > https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py > > File tools/dm_flags.py (right): > > > > > https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py#newc... > > tools/dm_flags.py:118: blacklist.extend('_ image gen_platform_opaque > > rgba32abf.bmp'.split(' ')) > > Doesn't the blacklist check for partial matches? e.g. could you cut these in > > half by just using "gen_platform" instead of "gen_platform_opaque" and > > "gen_platform_premul"? > > Will fix this.
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
I filed a bug for RAW/CG. CG handles RAW images a little differently than SkRawCodec. https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py File tools/dm_flags.py (right): https://codereview.chromium.org/1718273004/diff/300001/tools/dm_flags.py#newc... tools/dm_flags.py:118: blacklist.extend('_ image gen_platform_opaque rgba32abf.bmp'.split(' ')) On 2016/03/09 19:44:01, scroggo wrote: > Doesn't the blacklist check for partial matches? e.g. could you cut these in > half by just using "gen_platform" instead of "gen_platform_opaque" and > "gen_platform_premul"? Done.
The CQ bit was checked by msarett@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/1718273004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718273004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Patchset #9 (id:380001) has been deleted
The CQ bit was checked by msarett@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/1718273004/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718273004/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/09 20:55:45, msarett wrote: > I filed a bug for RAW/CG. CG handles RAW images a little differently than > SkRawCodec. Ah, so when you said "Stopped testing RAW images on image generator tests", you meant due to problems with the CG generator? I did not expect CG to support RAW (though perhaps I should have), so I was thinking the problem was on SkCodecImageGenerator. The code you've written looks like it will *also* skip the SkCodec version for RAW. That may be fine, unless we think it would catch a bug we would miss without it. lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718273004/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718273004/400001
Message was sent while issue was closed.
Description was changed from ========== Add an SkImageGeneratorCG This will serve as a replacement for SkImageDecoder_CG. BUG=skia:4914 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add an SkImageGeneratorCG This will serve as a replacement for SkImageDecoder_CG. BUG=skia:4914 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/1897631ebe417631ea7a046a2eb0995ab9d04539 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:400001) as https://skia.googlesource.com/skia/+/1897631ebe417631ea7a046a2eb0995ab9d04539 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
