Closed
Bug 777614
Opened 13 years ago
Closed 13 years ago
[Skia] Do another rebase
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: nrc, Assigned: gw280)
References
Details
Attachments
(8 files)
1.12 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
21.26 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
12.43 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
2.44 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
Apparently they've updated their gradient code, and we need that for bug 718849, which is blocking turning on Azure/Skia canvas, and that makes kittens cry.
Assignee | ||
Comment 1•13 years ago
|
||
Stop making kittens cry, you monster.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #661094 -
Flags: review?(ncameron)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #661095 -
Flags: review?(ncameron)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #661096 -
Flags: review?(ncameron)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #661097 -
Flags: review?(ncameron)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #661098 -
Flags: review?(ncameron)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #661099 -
Flags: review?(ncameron)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #661100 -
Flags: review?(ncameron)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #661101 -
Flags: review?(ncameron)
Reporter | ||
Updated•13 years ago
|
Attachment #661094 -
Flags: review?(ncameron) → review+
Reporter | ||
Comment 10•13 years ago
|
||
Comment on attachment 661094 [details] [diff] [review]
Bug 777614 - Re-add our SkUserConfig.h
Review of attachment 661094 [details] [diff] [review]:
-----------------------------------------------------------------
Do you need to update the patch which does this too, or is that somewhere else?
Reporter | ||
Updated•13 years ago
|
Attachment #661095 -
Flags: review?(ncameron) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #661098 -
Flags: review?(ncameron) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #661100 -
Flags: review?(ncameron) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #661101 -
Flags: review?(ncameron) → review+
Reporter | ||
Comment 11•13 years ago
|
||
Comment on attachment 661099 [details] [diff] [review]
Bug 777614 - Handle BGRX SourceSurfaces in Skia by converting them to BGRA
Review of attachment 661099 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with the casts changed
::: gfx/2d/HelpersSkia.h
@@ +115,5 @@
>
> +static inline void
> +ConvertBGRXToBGRA(unsigned char* aData, const IntSize &aSize, int32_t aStride)
> +{
> + uint32_t* pixel = (uint32_t*)aData;
nit: use reinterpret_cast
@@ +121,5 @@
> + for (int row = 0; row < aSize.height; ++row) {
> + for (int column = 0; column < aSize.width; ++column) {
> + pixel[column] |= 0xFF000000;
> + }
> + pixel += (aStride/4);
nit: brackets unnecessary, but I don't really object to leaving them in
::: gfx/2d/SourceSurfaceSkia.cpp
@@ +52,5 @@
> +
> + if (aFormat == FORMAT_B8G8R8X8) {
> + mBitmap.lockPixels();
> + // We have to manually set the A channel to be 255 as Skia doesn't understand BGRX
> + ConvertBGRXToBGRA((unsigned char*)mBitmap.getPixels(), aSize, aStride);
reinterpret_cast
Attachment #661099 -
Flags: review?(ncameron) → review+
Reporter | ||
Comment 12•13 years ago
|
||
Comment on attachment 661097 [details] [diff] [review]
Bug 777614 - Re-apply bug 687188 - Expand the gradient cache by 2 to store 0/1 colour stop values for clamping.
Review of attachment 661097 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm, but I don't deeply understand the code, I assume it has been reviewed before, given the name of the patch, if not we should ask someone who knows more for a review too.
Why hasn't this been upstreamed?
::: gfx/skia/src/effects/gradients/SkGradientShader.cpp
@@ +478,5 @@
> + SkColorGetB(fOrigColors[0]));
> + fCache32[kCache32ClampUpper] = SkPackARGB32(fCacheAlpha, SkColorGetR(fOrigColors[fColorCount - 1]),
> + SkColorGetG(fOrigColors[fColorCount - 1]),
> + SkColorGetB(fOrigColors[fColorCount - 1]));
> +
nit: new lines after fCacheAlpha
Attachment #661097 -
Flags: review?(ncameron) → review+
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 661096 [details] [diff] [review]
Bug 777614 - Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost
Review of attachment 661096 [details] [diff] [review]:
-----------------------------------------------------------------
Ugh, really? Is there not a better fix than reverting to some old code? Like fixing the new code? Do we not need to change a makefile or something to use this file rather than the original, or does that stay the same independent of the rebase? To clarify, are we just substituting older Skia code for newer Skia code here, we're not adding anything else etc.? I would prefer if that was apparent from the patch, for e.g., by changing the original file rather than adding a new one.
Assignee | ||
Comment 14•13 years ago
|
||
We already have this revert in the current skia that's in the tree; I'm just re-applying it. The "correct" fix is to use my Cairo SkFontHost, but that's not quite ready yet.
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to Nick Cameron [:nrc] from comment #12)
> Comment on attachment 661097 [details] [diff] [review]
> Bug 777614 - Re-apply bug 687188 - Expand the gradient cache by 2 to store
> 0/1 colour stop values for clamping.
>
> Review of attachment 661097 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> lgtm, but I don't deeply understand the code, I assume it has been reviewed
> before, given the name of the patch, if not we should ask someone who knows
> more for a review too.
>
> Why hasn't this been upstreamed?
I'm working on it, but it's taking a while, so this version is going into m-c for now until I can get it upstreamed.
Reporter | ||
Comment 16•13 years ago
|
||
Comment on attachment 661096 [details] [diff] [review]
Bug 777614 - Re-apply bug 719872 - Fix crash on Android by reverting to older FontHost
Review of attachment 661096 [details] [diff] [review]:
-----------------------------------------------------------------
Ugh, really? Is there not a better fix than reverting to some old code? Like fixing the new code? Do we not need to change a makefile or something to use this file rather than the original, or does that stay the same independent of the rebase? To clarify, are we just substituting older Skia code for newer Skia code here, we're not adding anything else etc.? I would prefer if that was apparent from the patch, for e.g., by changing the original file rather than adding a new one.
Attachment #661096 -
Flags: review?(ncameron) → review+
Assignee | ||
Comment 17•13 years ago
|
||
Landed on inbound. Kittens no longer crying.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab1d80a04f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d43bc68505
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3a9cb6edb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/70bf3735aa3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/73adb13034a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab27e89e7e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddbacc47f23
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa233c65e5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05f2097764c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e97b0f59229e
https://hg.mozilla.org/integration/mozilla-inbound/rev/81b2a9d68979
Comment 18•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/81b2a9d68979
https://hg.mozilla.org/mozilla-central/rev/e97b0f59229e
https://hg.mozilla.org/mozilla-central/rev/a05f2097764c
https://hg.mozilla.org/mozilla-central/rev/dfa233c65e5d
https://hg.mozilla.org/mozilla-central/rev/3ddbacc47f23
https://hg.mozilla.org/mozilla-central/rev/6ab27e89e7e8
https://hg.mozilla.org/mozilla-central/rev/73adb13034a8
https://hg.mozilla.org/mozilla-central/rev/70bf3735aa3d
https://hg.mozilla.org/mozilla-central/rev/2e3a9cb6edb9
https://hg.mozilla.org/mozilla-central/rev/21d43bc68505
https://hg.mozilla.org/mozilla-central/rev/dab1d80a04f4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 19•10 years ago
|
||
Was the removal of gfx/skia/update.sh intentional?
Assignee | ||
Comment 20•10 years ago
|
||
Yes. It was hard coded and very much out of date.
You need to log in
before you can comment on or make changes to this bug.
Description
•