Add skia::ConvertYUVAToRGBA function [chromium/src : main]
ccameron chromium (Gerrit)
ccameron chromium voted and added 2 comments![]( "Open in Gerrit")
Votes added by ccameron chromium
| Commit-Queue | +1 |
2 comments
// set based to the result of GetSkColorSpace().The `SkColorSpace` needs to be carried *somewhere*. I tried doing it separately, but that just ended up being messy. This also means that for RGBA video frames, the `SkPixmap` can just be used directly.
const SkPixmap& dst);The plan for this function is:
- If the src and dst color spaces are equal, then just use `libyuv`.
- Add a shader version when color space conversion is needed (if that indeed turns out to be more efficient).
- Add an option to allow treating the src and dst color spaces as equal if they're "close" (in particular, the sRGB, Rec709, and Apple-Rec709 transfer functions will be treated as the same).
Related details
- Dale Curtis
- Kaylee Lubick
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaylee Lubick (Gerrit)
Kaylee Lubick added 7 comments![]( "Open in Gerrit")
case SkYUVAInfo::PlaneConfig::kUnknown:Do we also want a default case in case the planeConfig is corrupted somehow?
DCHECK_EQ(src_pixmaps.size(), static_cast<size_t>(src_yuva_info.numPlanes()));We probably want this to be a CHECK_EQ to make sure we have enough data to read, yeah?
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);Can we make y a size_t too to avoid an implicit conversion? This one is safer (from inspection) so I'm less worried about it.
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);There's an implicit signed-to-unsigned conversion here. Can we add a cast to make that explicit and also check that we have a reasonable input for this (because we'll be doing 2^src_bit_depth later and I wouldn't want that to overflow or something)
const float s = 1023.f / 65535.f;nit: constexpr. Here and below
Can we add a testcase for the invalid yuva_info -> RGBA behavior?
}Can we add a test for the invalid yuva_info -> RGBA behavior? It doesn't have to use the same structure here since the assertions will be a bit different.
Related details
- Dale Curtis
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
return ColorSpace().GetAsFullRangeRGB().ToSkColorSpace();Should this be CompatRGBColorSpace?
const SkPixmap& dst);The plan for this function is:
- If the src and dst color spaces are equal, then just use `libyuv`.
- Add a shader version when color space conversion is needed (if that indeed turns out to be more efficient).
- Add an option to allow treating the src and dst color spaces as equal if they're "close" (in particular, the sRGB, Rec709, and Apple-Rec709 transfer functions will be treated as the same).
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ccameron chromium (Gerrit)
ccameron chromium added 10 comments![]( "Open in Gerrit")
return ColorSpace().GetAsFullRangeRGB().ToSkColorSpace();Should this be CompatRGBColorSpace?
I would add a separate `GetCompatSkColorSpace` for that. I put this next to `ColorSpace` to be emphatic about it.
const SkPixmap& dst);Dale CurtisThe plan for this function is:
- If the src and dst color spaces are equal, then just use `libyuv`.
- Add a shader version when color space conversion is needed (if that indeed turns out to be more efficient).
- Add an option to allow treating the src and dst color spaces as equal if they're "close" (in particular, the sRGB, Rec709, and Apple-Rec709 transfer functions will be treated as the same).
By libyuv, you just mean CopyPlane?
Things like `I420ToARGB`, etc.
case SkYUVAInfo::PlaneConfig::kUnknown:Do we also want a default case in case the planeConfig is corrupted somehow?
This only gets called if `SkYUVAInfo::isValid()` returns true, which requires that the plane config is not `kUnknown`. I added a testcase so we can see that it doesn't get hit.
DCHECK_EQ(src_pixmaps.size(), static_cast<size_t>(src_yuva_info.numPlanes()));We probably want this to be a CHECK_EQ to make sure we have enough data to read, yeah?
Changed this and the later ones to `CHECK`s.
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);There's an implicit signed-to-unsigned conversion here. Can we add a cast to make that explicit and also check that we have a reasonable input for this (because we'll be doing 2^src_bit_depth later and I wouldn't want that to overflow or something)
Changed the argument to a `size_t`.
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);Can we make y a size_t too to avoid an implicit conversion? This one is safer (from inspection) so I'm less worried about it.
Changed `w` and `h` to `size_t`.
nit: constexpr. Here and below
Done
Can we add a testcase for the invalid yuva_info -> RGBA behavior?
Added a test.
Can we add a test for the invalid yuva_info -> RGBA behavior? It doesn't have to use the same structure here since the assertions will be a bit different.
- Dale Curtis
- Kaylee Lubick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
return ColorSpace().GetAsFullRangeRGB().ToSkColorSpace();ccameron chromiumShould this be CompatRGBColorSpace?
I would add a separate `GetCompatSkColorSpace` for that. I put this next to `ColorSpace` to be emphatic about it.
I think this needs a bit more commentary on when to use it then, since it's not obvious even to me when you'd want to use Compat versus this one.
const SkPixmap& dst);Dale CurtisThe plan for this function is:
- If the src and dst color spaces are equal, then just use `libyuv`.
- Add a shader version when color space conversion is needed (if that indeed turns out to be more efficient).
- Add an option to allow treating the src and dst color spaces as equal if they're "close" (in particular, the sRGB, Rec709, and Apple-Rec709 transfer functions will be treated as the same).
ccameron chromiumBy libyuv, you just mean CopyPlane?
Things like `I420ToARGB`, etc.
Ah, I guess you meant in cases where primary, transfer are the same. You'd still need to make sure you choose the right matrix to give to libyuv then.
I was thinking you'd instead figure out how to write a SIMD optimized version using the Skia matrices though, so we don't end up with slightly different color spaces coming out on occasion.
Related details
- Kaylee Lubick
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaylee Lubick (Gerrit)
Kaylee Lubick voted and added 5 comments![]( "Open in Gerrit")
Votes added by Kaylee Lubick
| Code-Review | +1 |
5 comments
case SkYUVAInfo::PlaneConfig::kUnknown:ccameron chromiumDo we also want a default case in case the planeConfig is corrupted somehow?
This only gets called if `SkYUVAInfo::isValid()` returns true, which requires that the plane config is not `kUnknown`. I added a testcase so we can see that it doesn't get hit.
Acknowledged
DCHECK_EQ(src_pixmaps.size(), static_cast<size_t>(src_yuva_info.numPlanes()));ccameron chromiumWe probably want this to be a CHECK_EQ to make sure we have enough data to read, yeah?
Changed this and the later ones to `CHECK`s.
Acknowledged
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);ccameron chromiumCan we make y a size_t too to avoid an implicit conversion? This one is safer (from inspection) so I'm less worried about it.
Changed `w` and `h` to `size_t`.
Acknowledged
ReadYUVRow(src_pixmaps, src_yuva_info, src_bit_depth, y, src_row);ccameron chromiumThere's an implicit signed-to-unsigned conversion here. Can we add a cast to make that explicit and also check that we have a reasonable input for this (because we'll be doing 2^src_bit_depth later and I wouldn't want that to overflow or something)
Changed the argument to a `size_t`.
Acknowledged
}Can we add a test for the invalid yuva_info -> RGBA behavior? It doesn't have to use the same structure here since the assertions will be a bit different.
(I was able to add it in the same structure).
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |