Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
973563002:
Adding linear interpolation to rgb->yuv conversion (Closed)
DescriptionAdding linear interpolation to rgb->yuv conversion
When the UV planes are smaller than the Y plane, doing the upscaling in nearest mode was creating artefacts, so I changed it to use linear interpolation to fix the issue.
BUG=460380
Committed: https://skia.googlesource.com/skia/+/cd9d42c5167a50f1bf20e969343556d61354171b
Committed: https://skia.googlesource.com/skia/+/4ab3dbb636d07d03df1190c8a2b0a730e30e0d29
Patch Set 1 #Patch Set 2 : Switched YUV textures from Approx to Exact #
Total comments: 2
Patch Set 3 : Only use Exact when necessary #
Messages
Total messages: 29 (5 generated)
sugoi@chromium.org changed reviewers: + bsalomon@google.com
reed@google.com changed reviewers: + reed@google.com, scroggo@google.com
what does libjpeg do when we ask them to return RGB?
On 2015/03/02 21:29:24, reed1 wrote: > what does libjpeg do when we ask them to return RGB? There are some options available. One is do_block_smoothing(), which provides interpolation between blocks, and another one is do_fancy_upsampling(), which switches between the box filter (not fancy) and the triangle filter (fancy). From the doc: "The upsampling algorithm is linear interpolation between pixel centers, also known as a "triangle filter". This is a good compromise between speed and visual quality. The centers of the output pixels are 1/4 and 3/4 of the way between input pixel centers."
I thought that the u and v values were shared for a block. bilerp doesn't seem right to me.
On 2015/03/02 21:55:38, sugoi1 wrote: > On 2015/03/02 21:29:24, reed1 wrote: > > what does libjpeg do when we ask them to return RGB? > > There are some options available. One is do_block_smoothing(), which provides > interpolation between blocks, and another one is do_fancy_upsampling(), which > switches between the box filter (not fancy) and the triangle filter (fancy). > From the doc: > > "The upsampling algorithm is linear interpolation between pixel centers, also > known as a "triangle filter". This is a good compromise between speed and > visual quality. The centers of the output pixels are 1/4 and 3/4 of the way > between input pixel centers." Ah. What sampling do we get by default (as used today)?
On 2015/03/02 21:56:02, bsalomon wrote: > I thought that the u and v values were shared for a block. bilerp doesn't seem > right to me. I'm not sure, U and V planes are extracted at this point, they should no longer be in blocks.
On 2015/03/02 21:59:24, reed1 wrote: > On 2015/03/02 21:55:38, sugoi1 wrote: > > On 2015/03/02 21:29:24, reed1 wrote: > > > what does libjpeg do when we ask them to return RGB? > > > > There are some options available. One is do_block_smoothing(), which provides > > interpolation between blocks, and another one is do_fancy_upsampling(), which > > switches between the box filter (not fancy) and the triangle filter (fancy). > > From the doc: > > > > "The upsampling algorithm is linear interpolation between pixel centers, also > > known as a "triangle filter". This is a good compromise between speed and > > visual quality. The centers of the output pixels are 1/4 and 3/4 of the way > > between input pixel centers." > > Ah. What sampling do we get by default (as used today)? We use triangle everywhere except Android, where box is used.
What is the difference between do_block_smoothing() and do_fancy_upsampling() w/ triangle? We definitely need some tests in Skia code around this. Maybe a GM or, perhaps better, a unit test that draws using the yuv->rgb code path and does a readback and compares to the sw decode.
On 2015/03/02 22:25:58, bsalomon wrote: > What is the difference between do_block_smoothing() and do_fancy_upsampling() w/ > triangle? As I understand it, do_block_smoothing affects which pixels can be use while filtering. It allows the algorithm to smooth between the different blocks (possibly to avoid seams between blocks, like smoothing between the tiles of an image). I guess this is an option since each block will require information from its neighboring blocks to do so, which has a potential cost. do_fancy_upsampling merely changes the coefficients use on each pixel used while filtering, but I don't think it affect which pixels are used. When do_fancy_upsampling is false, all pixels have the same coefficient (so it's a box filter), otherwise it uses other coefficients for the triangle filter, but I think it only uses powers of 1/2, so that each pixel's value can be computed using shifts. > We definitely need some tests in Skia code around this. Maybe a GM or, perhaps > better, a unit test that draws using the yuv->rgb code path and does a readback > and compares to the sw decode. Sure, no problem. I don't think the CPU and GPU would yield the exact same result, though, if we want the GPU path to be fast. Visually, using linear filtering looks a lot like the fancy upsampling, on the test image given with this bug (while nearest looks completely different, as we would expect). Since almost all JPEG images we can find on the web are either YUV420 (1/2 w, 1/2 h) or YUV422 (1/2 w, full h), I'm not sure how much we can gain from fancier than linear filtering on the GPU VS how much it would cost to implement and render.
Thanks for those details. I didn't realize it was that tricky. I agree about tests. We do want to be fast, but if we are degrading the quality at all, we need to explicitly have reviewed that make the choice for that trade-off. I would feel best if we had *some* mode where the GPU generated effectively the same RGB (or better) in some mode, but then we could consider "faster/cheaper" modes as an option.
Perhaps unit testing is too tricky to get right. But I think we should at least have GMs that show the improvement made by the switch to bilerp for an image that looked poor before the CL.
On 2015/03/03 17:45:10, bsalomon wrote:
> Perhaps unit testing is too tricky to get right. But I think we should at
least
> have GMs that show the improvement made by the switch to bilerp for an image
> that looked poor before the CL.
So I've been looking at different existing gms and I think the existing
"colorwheel" test already exhibits this issue and looks clearly much better with
the fix. I don't know if this can be considered "good enough". Visually
speaking, the original gpu image using "nearest" filter looks quite a bit like
the 8888 image (it shows similar artefacts), but that comes from the following
issue: With the patch, the gpu image using "linear" actually looks much better
than the 8888 version, but that's somewhat misleading, because in skia's jpeg
decoder, there's a function called "turn_off_visual_optimizations()", which
essentially does:
cinfo->do_fancy_upsampling = 0;
cinfo->do_block_smoothing = 0;
Skia does this for speed reasons, but that means that jpeg images decoded by
skia look much crappier than jpeg images decoded by Blink's jpeg image decoded,
so the linear fix here makes the GPU decoded images look better than the CPU
decoded images.
On 2015/03/03 19:33:31, sugoi1 wrote: > On 2015/03/03 17:45:10, bsalomon wrote: > > Perhaps unit testing is too tricky to get right. But I think we should at > least > > have GMs that show the improvement made by the switch to bilerp for an image > > that looked poor before the CL. > > So I've been looking at different existing gms and I think the existing > "colorwheel" test already exhibits this issue and looks clearly much better with > the fix. I don't know if this can be considered "good enough". Visually > speaking, the original gpu image using "nearest" filter looks quite a bit like > the 8888 image (it shows similar artefacts), but that comes from the following > issue: With the patch, the gpu image using "linear" actually looks much better > than the 8888 version, but that's somewhat misleading, because in skia's jpeg > decoder, there's a function called "turn_off_visual_optimizations()", which > essentially does: > > cinfo->do_fancy_upsampling = 0; > cinfo->do_block_smoothing = 0; > > Skia does this for speed reasons, but that means that jpeg images decoded by > skia look much crappier than jpeg images decoded by Blink's jpeg image decoded, > so the linear fix here makes the GPU decoded images look better than the CPU > decoded images. That's interesting. If we already have a GM that shows the improvement then lgtm.
On 2015/03/03 19:33:31, sugoi1 wrote: > On 2015/03/03 17:45:10, bsalomon wrote: > > Perhaps unit testing is too tricky to get right. But I think we should at > least > > have GMs that show the improvement made by the switch to bilerp for an image > > that looked poor before the CL. > > So I've been looking at different existing gms and I think the existing > "colorwheel" test already exhibits this issue and looks clearly much better with > the fix. I don't know if this can be considered "good enough". Visually > speaking, the original gpu image using "nearest" filter looks quite a bit like > the 8888 image (it shows similar artefacts), but that comes from the following > issue: With the patch, the gpu image using "linear" actually looks much better > than the 8888 version, but that's somewhat misleading, because in skia's jpeg > decoder, there's a function called "turn_off_visual_optimizations()", which > essentially does: > > cinfo->do_fancy_upsampling = 0; > cinfo->do_block_smoothing = 0; > > Skia does this for speed reasons, but that means that jpeg images decoded by > skia look much crappier than jpeg images decoded by Blink's jpeg image decoded, > so the linear fix here makes the GPU decoded images look better than the CPU > decoded images. That function was written a long time ago, and the trade offs we made back then are necessarily the same we would make now. As we rewrite our decoders, we should probably be doing the same thing as Chrome (which it sounds like is to leave on fancy upsampling and block smoothing).
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/973563002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/cd9d42c5167a50f1bf20e969343556d61354171b
Message was sent while issue was closed.
I see that this landed. Regarding the colorwheel, have we compared its GPU quality against the HQ version of CPU (i.e. the way blink runs)? Seems like that was the issue. BTW - I agree that we should find a way to have skia bots run in the same manner as chrome, and we (scroggo) will work on a fix for that.
Message was sent while issue was closed.
On 2015/03/03 21:48:05, reed1 wrote: > Regarding the colorwheel, have we compared its GPU quality against the HQ > version of CPU (i.e. the way blink runs)? Seems like that was the issue. Locally, it was very hard to distinguish between both visually, but I'm pretty sure they weren't numerically equivalent. I want to have a look at the perf telemetry bots for the decoding test to see if switching U and V planes to linear will cause a noticeable slowdown or not (mostly on mobile platforms), so I thought it would be nice to have this in ASAP. It is visually pretty clear that linear interpolation looks much better than nearest and I'd like to find a convincing case where CPU (in Blink) and GPU (with linear) are significantly different, but I don't have one just yet.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/977133002/ by joshualitt@google.com. The reason for reverting is: Speculative revert to see if this unblocks the deps roll.
Ok, so the issue was a bit weird, but this may fix it. Here's what it was: By using GrContext::kApprox_ScratchTexMatch instead of GrContext::kExact_ScratchTexMatch and by using GL_LINEAR interpolation mode, it was possible to get weirdness at the borders. That was picked up by the WebRtcBrowserTest.CanSetupVideoCallAndDisableLocalVideo test, because it checks if a frame is black post YUV decoding, pixel by pixel, here: waitForBlackVideo(): https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/... isVideoBlack(): https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/... Hopefully, switching from GrContext::kApprox_ScratchTexMatch to GrContext::kExact_ScratchTexMatch should prevent this issue from happening.
https://codereview.chromium.org/973563002/diff/20001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/973563002/diff/20001/src/gpu/SkGr.cpp#newcode369 src/gpu/SkGr.cpp:369: ctx->refScratchTexture(yuvDesc, GrContext::kExact_ScratchTexMatch)); Maybe only do this if we will use bilerp? Otherwise, lgtm.
https://codereview.chromium.org/973563002/diff/20001/src/gpu/SkGr.cpp File src/gpu/SkGr.cpp (right): https://codereview.chromium.org/973563002/diff/20001/src/gpu/SkGr.cpp#newcode369 src/gpu/SkGr.cpp:369: ctx->refScratchTexture(yuvDesc, GrContext::kExact_ScratchTexMatch)); On 2015/03/05 16:13:05, bsalomon wrote: > Maybe only do this if we will use bilerp? Otherwise, lgtm. Done.
The CQ bit was checked by sugoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/973563002/#ps40001 (title: "Only use Exact when necessary")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/973563002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/4ab3dbb636d07d03df1190c8a2b0a730e30e0d29 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
