Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1013743003:
Adding premul and 565 swizzles for bmp: (Closed)
DescriptionAdding swizzles for bmp:
We now support kN32 and kRGB_565 color types.
Additionally, we support premul, unpremul, and opaque alpha types.
Unpremul is currently untested as we cannot currently draw to unpremul.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/eed039b5ffbdff958053ac80b09451ad6caa1787
Patch Set 1 #Patch Set 2 : Rebasing on master #Patch Set 3 : Rebasing to master again #Patch Set 4 : Clean up before public code review #
Total comments: 33
Patch Set 5 : Moved color table checks to onGetPixels, Disabled swizzles non-opaque swizzles to 565 #
Total comments: 13
Patch Set 6 : Removed bytesRead param, build color table on stack #
Total comments: 6
Patch Set 7 : Added profile type check #
Total comments: 16
Patch Set 8 : Made MaskSwizzler::next consistent with Swizzler #
Total comments: 1
Patch Set 9 : Trybot fixes #
Total comments: 1
Messages
Total messages: 28 (6 generated)
Adding swizzles for bmp:
We now support kN32 and kRGB_565 color types.
Additionally, we support premul, unpremul, and opaque alpha types.
Unpremul is currently untested as we cannot currently draw to unpremul.
Sorry, this is another large chunk of code. Good news is most of the volume in
terms of lines is from additional row procs. I wonder if there is a way to
share code on row procs while keeping them fast and light.
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm...
File src/codec/SkCodec_libbmp.cpp (right):
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm...
src/codec/SkCodec_libbmp.cpp:593: uint8_t* srcBuffer(SkNEW_ARRAY(uint8_t,
height*rowBytes));
I know that it is preferred to use an SkAutoTDeleteArray. The reason I am not
using this template is that I need to move row by row through the buffer. As
far as I could tell this type of pointer arithmetic would be messy using an
AutoTDelete type. Let me know if there is an option that I am missing.
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm...
src/codec/SkCodec_libbmp.cpp:639: newInfo, fMasks, fBitsPerPixel);
Increase indent
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:355: proc = &swizzle_bgra_to_565_premul;
If the image is not opaque, I always premultiply before swizzling to 565. Let
me know if this is not a valid decision.
msarett@google.com changed reviewers: + scroggo@google.com
Adding swizzles for bmp:
We now support kN32 and kRGB_565 color types.
Additionally, we support premul, unpremul, and opaque alpha types.
Unpremul is currently untested as we cannot currently draw to unpremul.
Sorry, this is another large chunk of code. Good news is most of the volume in
terms of lines is from additional row procs. I wonder if there is a way to
share code on row procs while keeping them fast and light.
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:397: const uint32_t maxColors = 1 << bitsPerPixel; Could you move all of this into SkBmpCodec's onGetPixels? Here are the pros and cons, as far as I see it: pro: Don't need to pass as many parameters to the creation of the codec (AFAICT). Instead, we can declare these variables where they're used. con: Maybe we won't be able to do the bytesRead > offset check here (unless we duplicate work). https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:518: colorTable = SkNEW_ARRAY(SkPMColor, maxColors); colorTable will leak if we return NULL below. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:538: colorTable[i] = SkPackARGB32NoCheck(alpha, red, green, nit: no need to update the variable alpha. You can just put 0xFF inline here. I'd be surprised if compiler created an extra assignment, but I think it's more readable with inlining. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:593: uint8_t* srcBuffer(SkNEW_ARRAY(uint8_t, height*rowBytes)); On 2015/03/16 20:21:53, msarett wrote: > I know that it is preferred to use an SkAutoTDeleteArray. The reason I am not > using this template is that I need to move row by row through the buffer. As > far as I could tell this type of pointer arithmetic would be messy using an > AutoTDelete type. Let me know if there is an option that I am missing. You should definitely use some sort of auto deleter. (FWIW, in PNG I use an SkAutoMalloc, and cast the return value of ::get() to the type I want. I don't know that one is better than the other.) I don't see the issue with pointer arithmetic. You already use the raw pointer srcRow; you can do pointer arithmetic on that. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:631: srcRow = SkTAddOffset<uint8_t>(srcRow, rowBytes); How did this work before? https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:637: const SkImageInfo& newInfo = dstInfo.makeAlphaType(kOpaque_SkAlphaType); nit: Maybe this should be opaqueInfo? Or maskInfo? https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:638: SkMaskSwizzler* newSwizzler = SkMaskSwizzler::CreateMaskSwizzler( nit: Maybe name this maskSwizzler? https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:638: SkMaskSwizzler* newSwizzler = SkMaskSwizzler::CreateMaskSwizzler( This will leak. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:81: bool createColorTable(const SkAlphaType alphaType); nit: We typically do not mark enum parameters as const (only reference/pointer parameters to objects/structs). https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:155: SkAutoTDeleteArray<SkPMColor> fColorTable; // owned, unpremul This is no longer necessarily unpremul. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:156: const uint32_t fNumColors; FWIW, you could use an SkColorTable which holds both the colors and a count. (It only goes up to 256, but I imagine you don't have more than that?) https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:160: const uint32_t fRemainingBytes; It seems like the meaning of fRemainingBytes has changed. Maybe it should be something like fRLEBytes? https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:307: proc = &swizzle_mask16_to_565_premul; 565 is only opaque, so we 565 premul does not make sense. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:355: proc = &swizzle_bgra_to_565_premul; On 2015/03/16 20:21:53, msarett wrote: > If the image is not opaque, I always premultiply before swizzling to 565. Let > me know if this is not a valid decision. If the image is 565, we should not allow using premul/unpremul, since that we cannot represent non-opaque alpha in 565
Once again, thanks for your review. I moved all of the color table code to onGetPixels and disabled 565 swizzles for non-opaque images. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:397: const uint32_t maxColors = 1 << bitsPerPixel; On 2015/03/17 13:27:22, scroggo wrote: > Could you move all of this into SkBmpCodec's onGetPixels? > > Here are the pros and cons, as far as I see it: > > pro: > Don't need to pass as many parameters to the creation of the codec (AFAICT). > Instead, we can declare these variables where they're used. > > con: > Maybe we won't be able to do the bytesRead > offset check here (unless we > duplicate work). Doing these checks in onGetPixels actually causes us to need another parameter (bytesRead). However, I definitely see the pro of keeping all of the color table related work in one place. Additionally, we actually have less duplicated code. The reason I chose the original design was that it would be nice to identify invalid inputs in NewFromStream rather than onGetPixels. However, I feel that this error case is rare enough that it is sufficient to catch the error later. I agree making this change is a net improvement and have done so. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:518: colorTable = SkNEW_ARRAY(SkPMColor, maxColors); On 2015/03/17 13:27:22, scroggo wrote: > colorTable will leak if we return NULL below. Done. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:538: colorTable[i] = SkPackARGB32NoCheck(alpha, red, green, On 2015/03/17 13:27:22, scroggo wrote: > nit: no need to update the variable alpha. You can just put 0xFF inline here. > I'd be surprised if compiler created an extra assignment, but I think it's more > readable with inlining. Done. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:593: uint8_t* srcBuffer(SkNEW_ARRAY(uint8_t, height*rowBytes)); On 2015/03/17 13:27:22, scroggo wrote: > On 2015/03/16 20:21:53, msarett wrote: > > I know that it is preferred to use an SkAutoTDeleteArray. The reason I am not > > using this template is that I need to move row by row through the buffer. As > > far as I could tell this type of pointer arithmetic would be messy using an > > AutoTDelete type. Let me know if there is an option that I am missing. > > You should definitely use some sort of auto deleter. (FWIW, in PNG I use an > SkAutoMalloc, and cast the return value of ::get() to the type I want. I don't > know that one is better than the other.) > > I don't see the issue with pointer arithmetic. You already use the raw pointer > srcRow; you can do pointer arithmetic on that. Agreed, not sure what I was thinking. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:631: srcRow = SkTAddOffset<uint8_t>(srcRow, rowBytes); On 2015/03/17 13:27:22, scroggo wrote: > How did this work before? We used to overwrite the same row buffer for each row that we decoded. Now we store all the rows in a src buffer in case we need to redo the decode. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:637: const SkImageInfo& newInfo = dstInfo.makeAlphaType(kOpaque_SkAlphaType); On 2015/03/17 13:27:22, scroggo wrote: > nit: Maybe this should be opaqueInfo? Or maskInfo? Done. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:638: SkMaskSwizzler* newSwizzler = SkMaskSwizzler::CreateMaskSwizzler( On 2015/03/17 13:27:22, scroggo wrote: > This will leak. Yeah it looks like it. I think this is not the only place. I have fixed it. I went with opaqueSwizzler as the name. I renamed the first swizzler maskSwizzler. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbmp.h File src/codec/SkCodec_libbmp.h (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:81: bool createColorTable(const SkAlphaType alphaType); On 2015/03/17 13:27:22, scroggo wrote: > nit: We typically do not mark enum parameters as const (only reference/pointer > parameters to objects/structs). Done. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:155: SkAutoTDeleteArray<SkPMColor> fColorTable; // owned, unpremul On 2015/03/17 13:27:22, scroggo wrote: > This is no longer necessarily unpremul. Sorry, I missed that. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:156: const uint32_t fNumColors; On 2015/03/17 13:27:22, scroggo wrote: > FWIW, you could use an SkColorTable which holds both the colors and a count. (It > only goes up to 256, but I imagine you don't have more than that?) Unfortuntately, using SkColorTable does not allow me to get rid of this field. The only public constructor takes a const pointer, so I would not be able to initialize the SkColorTable until I finish constructing the color table. But I think it does make sense to use SkColorTable given that I have a color table, so I will. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.h:160: const uint32_t fRemainingBytes; On 2015/03/17 13:27:22, scroggo wrote: > It seems like the meaning of fRemainingBytes has changed. Maybe it should be > something like fRLEBytes? Done. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkMaskSwizzle... File src/codec/SkMaskSwizzler.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkMaskSwizzle... src/codec/SkMaskSwizzler.cpp:307: proc = &swizzle_mask16_to_565_premul; On 2015/03/17 13:27:22, scroggo wrote: > 565 is only opaque, so we 565 premul does not make sense. That's good news. Reduces the number of cases and row procs. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:355: proc = &swizzle_bgra_to_565_premul; On 2015/03/17 13:27:23, scroggo wrote: > On 2015/03/16 20:21:53, msarett wrote: > > If the image is not opaque, I always premultiply before swizzling to 565. Let > > me know if this is not a valid decision. > > If the image is 565, we should not allow using premul/unpremul, since that we > cannot represent non-opaque alpha in 565 Done. I will disallow in conversion possible and remove these cases.
https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:397: const uint32_t maxColors = 1 << bitsPerPixel; On 2015/03/17 16:54:05, msarett wrote: > On 2015/03/17 13:27:22, scroggo wrote: > > Could you move all of this into SkBmpCodec's onGetPixels? > > > > Here are the pros and cons, as far as I see it: > > > > pro: > > Don't need to pass as many parameters to the creation of the codec (AFAICT). > > Instead, we can declare these variables where they're used. > > > > con: > > Maybe we won't be able to do the bytesRead > offset check here (unless we > > duplicate work). > > Doing these checks in onGetPixels actually causes us to need another parameter > (bytesRead). I think we can get around this... How about this: continue to pass "offset - bytesRead" to the constructor as the offset. In createColorTable, instead of checking: if (fBytesRead + colorBytes > fOffset) check if (colorBytes > fOffset) Then you can skip fOffset - colorBytes. I want to get rid of fBytesRead for a few reasons: - It's only needed in one place - For the rest of an SkBmpCodec's lifetime, it is no longer true - SkBmpCodec has lots of member variables already - The constructor of SkBmpCodec has lots of parameters. - I think we can do without it. https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:631: srcRow = SkTAddOffset<uint8_t>(srcRow, rowBytes); On 2015/03/17 16:54:05, msarett wrote: > On 2015/03/17 13:27:22, scroggo wrote: > > How did this work before? > > We used to overwrite the same row buffer for each row that we decoded. Now we > store all the rows in a src buffer in case we need to redo the decode. Ah, of course! https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:25: premul_and_unpremul(dst.alphaType(), src.alphaType()); Oh, clever. You don't need to check this in both orders, since we know the src could only have been unpremul or opaque. I naively checked in both directions for PNG.... Given that, it might be clearer to remove premul_and_unpremul and just inline the code (both here and in PNG). https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:249: memset(&inputMasks, 0, 4*sizeof(uint32_t)); nit: Maybe this should be sizeof(SkMasks::InputMasks)? Probably fine as is, but if we changed the members of InputMasks, this might not zero them out properly. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:360: const int RLEBytes = totalBytes - offset; size_t https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:566: SkNEW_ARGS(SkColorTable, (colorTable.detach(), maxColors))); Unfortunately, this constructor doesn't do what you think it does. (Even worse, it's not documented in any way...). Instead of taking ownership, it makes a copy of the passed in colors. Take a look at what SkPngCodec does - it stack allocates 256 colors and then copies them into a new SkColorTable. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:670: dstRow[x] = fColorTable.get()[0][index]; Why do you need [0]? Does this get around the fact that get() returns a pointer, which is then treated as an array? As an alternative (maybe equally ugly...), I think you can use fColorTable->operator[](index) https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:960: fColorTable.get()->readColors(), dstInfo, dst, dstRowBytes, SkAutoTDelete implements operator->, so you can drop the .get() here, and treat it as if it were a raw pointer (that's what allows the ->operator[]() trick I mentioned above).
Removed bytesRead parameter Created color table on stack before copying into SkColorTable https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/60001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:397: const uint32_t maxColors = 1 << bitsPerPixel; On 2015/03/17 20:02:58, scroggo wrote: > On 2015/03/17 16:54:05, msarett wrote: > > On 2015/03/17 13:27:22, scroggo wrote: > > > Could you move all of this into SkBmpCodec's onGetPixels? > > > > > > Here are the pros and cons, as far as I see it: > > > > > > pro: > > > Don't need to pass as many parameters to the creation of the codec (AFAICT). > > > Instead, we can declare these variables where they're used. > > > > > > con: > > > Maybe we won't be able to do the bytesRead > offset check here (unless we > > > duplicate work). > > > > Doing these checks in onGetPixels actually causes us to need another parameter > > (bytesRead). > > I think we can get around this... > > How about this: > > continue to pass "offset - bytesRead" to the constructor as the offset. > > In createColorTable, instead of checking: > if (fBytesRead + colorBytes > fOffset) > > check > > if (colorBytes > fOffset) > > Then you can skip fOffset - colorBytes. > > I want to get rid of fBytesRead for a few reasons: > - It's only needed in one place > - For the rest of an SkBmpCodec's lifetime, it is no longer true > - SkBmpCodec has lots of member variables already > - The constructor of SkBmpCodec has lots of parameters. > - I think we can do without it. Yes great! That's a much better way to do it. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:25: premul_and_unpremul(dst.alphaType(), src.alphaType()); On 2015/03/17 20:02:58, scroggo wrote: > Oh, clever. You don't need to check this in both orders, since we know the src > could only have been unpremul or opaque. I naively checked in both directions > for PNG.... > > Given that, it might be clearer to remove premul_and_unpremul and just inline > the code (both here and in PNG). Agreed. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:249: memset(&inputMasks, 0, 4*sizeof(uint32_t)); On 2015/03/17 20:02:58, scroggo wrote: > nit: Maybe this should be sizeof(SkMasks::InputMasks)? Probably fine as is, but > if we changed the members of InputMasks, this might not zero them out properly. Done. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:360: const int RLEBytes = totalBytes - offset; On 2015/03/17 20:02:58, scroggo wrote: > size_t Done. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:566: SkNEW_ARGS(SkColorTable, (colorTable.detach(), maxColors))); On 2015/03/17 20:02:58, scroggo wrote: > Unfortunately, this constructor doesn't do what you think it does. (Even worse, > it's not documented in any way...). Instead of taking ownership, it makes a copy > of the passed in colors. > > Take a look at what SkPngCodec does - it stack allocates 256 colors and then > copies them into a new SkColorTable. Yeah I had no idea. This should be better. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:670: dstRow[x] = fColorTable.get()[0][index]; On 2015/03/17 20:02:58, scroggo wrote: > Why do you need [0]? > > Does this get around the fact that get() returns a pointer, which is then > treated as an array? > > As an alternative (maybe equally ugly...), I think you can use > > fColorTable->operator[](index) Yeah I think that's a little better. I kept going back and forth between [0] and (*(...)) both of which I didn't like. https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:960: fColorTable.get()->readColors(), dstInfo, dst, dstRowBytes, On 2015/03/17 20:02:58, scroggo wrote: > SkAutoTDelete implements operator->, so you can drop the .get() here, and treat > it as if it were a raw pointer (that's what allows the ->operator[]() trick I > mentioned above). Done.
https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/80001/src/codec/SkCodec_libbm... src/codec/SkCodec_libbmp.cpp:670: dstRow[x] = fColorTable.get()[0][index]; On 2015/03/18 13:02:52, msarett wrote: > On 2015/03/17 20:02:58, scroggo wrote: > > Why do you need [0]? > > > > Does this get around the fact that get() returns a pointer, which is then > > treated as an array? > > > > As an alternative (maybe equally ugly...), I think you can use > > > > fColorTable->operator[](index) > > Yeah I think that's a little better. > > I kept going back and forth between [0] and (*(...)) both of which I didn't > like. Haha, yeah, it's an unfortunate bit of ambiguity that I wish had a better fix. https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:498: SkPMColor colorTable[maxColors]; Maybe this works, but I think it would be better to put colorTable in the same scope as colorTablePtr. That said, once you move colorTable into the main scope, it looks like you could just use colorTable the whole time, and never bother with colorTablePtr. (FWIW, the PngCodec has a separate pointer because it uses pointer arithmetic instead of indexing, whereas your code uses an index. I'm not sure whether the original author of the PNG code uses pointer arithmetic as a matter of preference or for some perceived benefit.) https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libp... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libp... src/codec/SkCodec_libpng.cpp:355: if (dst.profileType() != src.profileType()) { BMP doesn't make this check, but it should. (I'm guessing all of BMP will be linear?)
Minor fixes https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:498: SkPMColor colorTable[maxColors]; On 2015/03/18 13:33:20, scroggo wrote: > Maybe this works, but I think it would be better to put colorTable in the same > scope as colorTablePtr. > > That said, once you move colorTable into the main scope, it looks like you could > just use colorTable the whole time, and never bother with colorTablePtr. > > (FWIW, the PngCodec has a separate pointer because it uses pointer arithmetic > instead of indexing, whereas your code uses an index. I'm not sure whether the > original author of the PNG code uses pointer arithmetic as a matter of > preference or for some perceived benefit.) The reason I set it up this wasn't really because of png. I just wanted to perform the stack allocation after maxColors was calculated. That said, the calculation of maxColors can also be hoisted out of the loop. Another alternative would be to always allocate space for 256 entries. I doubt this would be too bad, but I am trying to avoid it given that many bmps don't have a color table or use a color table with 1, 2, or 4 bit indices. https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libp... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libp... src/codec/SkCodec_libpng.cpp:355: if (dst.profileType() != src.profileType()) { On 2015/03/18 13:33:20, scroggo wrote: > BMP doesn't make this check, but it should. (I'm guessing all of BMP will be > linear?) Done.
https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:498: SkPMColor colorTable[maxColors]; On 2015/03/18 14:09:12, msarett wrote: > On 2015/03/18 13:33:20, scroggo wrote: > > Maybe this works, but I think it would be better to put colorTable in the same > > scope as colorTablePtr. > > > > That said, once you move colorTable into the main scope, it looks like you > could > > just use colorTable the whole time, and never bother with colorTablePtr. > > > > (FWIW, the PngCodec has a separate pointer because it uses pointer arithmetic > > instead of indexing, whereas your code uses an index. I'm not sure whether the > > original author of the PNG code uses pointer arithmetic as a matter of > > preference or for some perceived benefit.) > > The reason I set it up this wasn't really because of png. I just wanted to > perform the stack allocation after maxColors was calculated. That said, the > calculation of maxColors can also be hoisted out of the loop. > > Another alternative would be to always allocate space for 256 entries. I doubt > this would be too bad, but I am trying to avoid it given that many bmps don't > have a color table or use a color table with 1, 2, or 4 bit indices. What I meant to say was "moved out of the if statement", not "hoisted out of the loop".
https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/100001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:498: SkPMColor colorTable[maxColors]; On 2015/03/18 14:09:12, msarett wrote: > On 2015/03/18 13:33:20, scroggo wrote: > > Maybe this works, but I think it would be better to put colorTable in the same > > scope as colorTablePtr. > > > > That said, once you move colorTable into the main scope, it looks like you > could > > just use colorTable the whole time, and never bother with colorTablePtr. > > > > (FWIW, the PngCodec has a separate pointer because it uses pointer arithmetic > > instead of indexing, whereas your code uses an index. I'm not sure whether the > > original author of the PNG code uses pointer arithmetic as a matter of > > preference or for some perceived benefit.) > > The reason I set it up this wasn't really because of png. I just wanted to > perform the stack allocation after maxColors was calculated. That said, the > calculation of maxColors can also be hoisted out of the loop. > > Another alternative would be to always allocate space for 256 entries. I doubt > this would be too bad, but I am trying to avoid it given that many bmps don't > have a color table or use a color table with 1, 2, or 4 bit indices. I think that would be fine (it's what we do for PNG). This approach is fine, too, though. FWIW, the problem with the approach in patch set 6 is that colorTablePtr points to an array that goes out of scope before you use colorTablePtr, so it's not guaranteed to still be valid. (It probably will be, though, leading to subtle bugs that don't reproduce consistently.) https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:416: // set color type to kN32_SkColorType because that should be the default This doesn't need to be fixed in this CL, but I talked to Mike about this, and he pointed out that it should not be the default, but whatever best represents the data. So for an index bitmap, I think the logical one would be kIndex8 (even though it's not a perfect match). For a 555, I think 565 is our closest match. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:500: uint32_t maxColors = fBitsPerPixel <= 8 ? 1 << fBitsPerPixel : 0; nit: const https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:504: colorBytes = fNumColors * fBytesPerColor; Is it possible for fNumColors to be greater than maxColors? If so, shouldn't we stop at the min(fNumColors, maxColors)? https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:532: colorTable[i] = SkPreMultiplyARGB(alpha, red, green, nit: We could handle both premul and unpremul with the same code, by choosing which function to call outside the loop (pack vs premultiply). https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:585: // Allocate a buffer large enough to hold the full input stream nit: this holds the whole image, not the whole stream. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:619: SkSwizzler::ResultAlpha r = maskSwizzler->next(dstRow, srcRow); This is confusing. The second parameter for SkMaskSwizzler->next() is the src row, but the second parameter for SkSwizzler->next() is a y value. Can we unify them? https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:623: dstRow = SkTAddOffset<SkPMColor>(dstRow, delta); This isn't new to this CL, but if delta is negative, does this work? SkTAddOffset takes a size_t parameter. Does it not do an implicit cast?
Made MaskSwizzler::next consistent with Swizzler https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:416: // set color type to kN32_SkColorType because that should be the default On 2015/03/18 14:59:32, scroggo wrote: > This doesn't need to be fixed in this CL, but I talked to Mike about this, and > he pointed out that it should not be the default, but whatever best represents > the data. > > So for an index bitmap, I think the logical one would be kIndex8 (even though > it's not a perfect match). For a 555, I think 565 is our closest match. That's a good point, and something we should probably improve/optimize for down the line. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:500: uint32_t maxColors = fBitsPerPixel <= 8 ? 1 << fBitsPerPixel : 0; On 2015/03/18 14:59:32, scroggo wrote: > nit: const Done. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:504: colorBytes = fNumColors * fBytesPerColor; On 2015/03/18 14:59:32, scroggo wrote: > Is it possible for fNumColors to be greater than maxColors? If so, shouldn't we > stop at the min(fNumColors, maxColors)? There was code that fixes this possibility that seems to have disappeared in all of the refactoring. That's a big issue, glad you noticed it. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:532: colorTable[i] = SkPreMultiplyARGB(alpha, red, green, On 2015/03/18 14:59:32, scroggo wrote: > nit: We could handle both premul and unpremul with the same code, by choosing > which function to call outside the loop (pack vs premultiply). Maybe this is better? https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:585: // Allocate a buffer large enough to hold the full input stream On 2015/03/18 14:59:32, scroggo wrote: > nit: this holds the whole image, not the whole stream. Done. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:619: SkSwizzler::ResultAlpha r = maskSwizzler->next(dstRow, srcRow); On 2015/03/18 14:59:32, scroggo wrote: > This is confusing. The second parameter for SkMaskSwizzler->next() is the src > row, but the second parameter for SkSwizzler->next() is a y value. Can we unify > them? Yes I will unify. And I think this will fix the issue below as well. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:623: dstRow = SkTAddOffset<SkPMColor>(dstRow, delta); On 2015/03/18 14:59:32, scroggo wrote: > This isn't new to this CL, but if delta is negative, does this work? > SkTAddOffset takes a size_t parameter. Does it not do an implicit cast? It does work. But it seems that this was by luck more than anything else. I will change this.
https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:619: SkSwizzler::ResultAlpha r = maskSwizzler->next(dstRow, srcRow); On 2015/03/18 14:59:32, scroggo wrote: > This is confusing. The second parameter for SkMaskSwizzler->next() is the src > row, but the second parameter for SkSwizzler->next() is a y value. Can we unify > them? I know I told you that SkSwizzler::next() should take a y value, but I'm in the process of writing the scanline decoder, and it could use a pointer as well. It also seems that since the original SkSwizzler::next() takes the following parameter: (src) we should make the others (SkMaskSwizzler::next and SkSwizzler::next) start with src: (src, dst) This is a big change, and should probably be left for a different CL.
(src, y) are the current parameters to next. Happy to put off the possibility of using (src, dst) for a later CL. https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/120001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:619: SkSwizzler::ResultAlpha r = maskSwizzler->next(dstRow, srcRow); On 2015/03/18 16:02:06, scroggo wrote: > On 2015/03/18 14:59:32, scroggo wrote: > > This is confusing. The second parameter for SkMaskSwizzler->next() is the src > > row, but the second parameter for SkSwizzler->next() is a y value. Can we > unify > > them? > > I know I told you that SkSwizzler::next() should take a y value, but I'm in the > process of writing the scanline decoder, and it could use a pointer as well. > > It also seems that since the original SkSwizzler::next() takes the following > parameter: > > (src) > > we should make the others (SkMaskSwizzler::next and SkSwizzler::next) start with > src: > > (src, dst) > > This is a big change, and should probably be left for a different CL. I would agree that src should always be the first parameter. I changed to use (src, y) in the last CL, and I think it makes everything cleaner. I agree that a big change to (src, dst) would fit better in a different CL. It's a good option to have moving forward.
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/1013743003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)
The CQ bit was checked by msarett@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/1013743003/#ps160001 (title: "Trybot fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1013743003/160001
https://codereview.chromium.org/1013743003/diff/140001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/140001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:501: SkPMColor colorTable[maxColors]; I thought this wasn't allowed, but it worked on my linux machine so I just accepted it... https://codereview.chromium.org/1013743003/diff/160001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1013743003/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:573: fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorTable, maxColors))); It's sort of weird that we have may create an empty color table (for fBitsPerPixel > 8). But probably okay.
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/eed039b5ffbdff958053ac80b09451ad6caa1787
Message was sent while issue was closed.
Yeah I was pleasantly surprised when that compiled for me, but I guess it wasn't destined to get by the trybots. I'm not too thrilled about the idea of there always being an empty color table. AFAICT the alternate options are: ***Scrap the use of SkColorTable and go back to SkPMColor* ***Allocate the "under construction" color table on the heap before initializing SkColorTable (which will result in memory copy) ***Clever idea that Leon comes up with that I didn't think of I'm happy to consider any of these alternatives if you think it's better than the current design.
Message was sent while issue was closed.
On 2015/03/18 18:21:27, msarett wrote: > Yeah I was pleasantly surprised when that compiled for me, but I guess it wasn't > destined to get by the trybots. I'm not too thrilled about the idea of there > always being an empty color table. > > AFAICT the alternate options are: > ***Scrap the use of SkColorTable and go back to SkPMColor* > ***Allocate the "under construction" color table on the heap before initializing > SkColorTable (which will result in memory copy) > ***Clever idea that Leon comes up with that I didn't think of > > I'm happy to consider any of these alternatives if you think it's better than > the current design. One option I thought about was to only call createColorTable if fBitsPerPixel <= 8. I don't see a clean way to do that and then do the skip (which is currently part of createColorTable) though. Others: 1) create the color table inside the if fBitsPerPixel <= 8 block. We'll have the odd behavior that we may actually create the color table and return false, but I think it's okay to create the unneeded object for that failure case. 2) only create the color table if maxColors > 0
Message was sent while issue was closed.
scroggo@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
One last after the fact comment: Mike pointed out that we should not do any conversion down to 565, since we'll lose color information. Rather than perform dithering for the client, we can let them do it after the fact if they want to. If an image is encoded as 565 (or perhaps as 555, as in the BMP case), we can decode to 565. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
