coffee/libyuv
1
0
Fork 0
mirror of https://chromium.googlesource.com/libyuv/libyuv synced 2026-11-10 07:09:50 +08:00
Code Issues Packages Projects Releases Wiki Activity
libyuv/8dabf898eeee2e08d41666ee24533da35f4dcee7
Gerrit User 1115898 c1ed85027a Update patch set 6 
Patch Set 6:

(1 comment)

Patch-set: 6
Attention: {"person_ident":"Gerrit User 1115898 \u003c1115898@3ce6091f-6c88-37e8-8c75-72f92ae8dfba\u003e","operation":"REMOVE","reason":"removed on reply"}
2026-10-14 23:01:50 -07:00

250 lines
9.0 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "df11df10_747fe48c",
"filename": "/COMMIT_MSG",
"patchSetId": 4
},
"lineNbr": 31,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T23:09:21Z",
"side": 1,
"message": "Optional: add the following duplicate issue:\n\n```\nBug: chromium:546841026\n```",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "466cc5e2_8804b5c7",
"filename": "/COMMIT_MSG",
"patchSetId": 4
},
"lineNbr": 31,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T04:05:30Z",
"side": 1,
"message": "Done",
"parentUuid": "df11df10_747fe48c",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "ea7b6741_a4828652",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T22:59:39Z",
"side": 1,
"message": "LGTM. Thanks for the fix!",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "3019a141_74e1382a",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 947,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T22:59:39Z",
"side": 1,
"message": "Frank: Do you know why `max_y` is defined as `src_height \u003c\u003c 16` in only three places but defined as `(src_height - 1) \u003c\u003c 16` in all other places?",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "4122bc4c_2330d67a",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 947,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T04:05:30Z",
"side": 1,
"message": "my quick guess would be vertical subsampling where y+1 is also used.\nif it gets flagged as a bug we should sort that out",
"parentUuid": "3019a141_74e1382a",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "9a1b23b1_63aa6354",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 947,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T05:00:46Z",
"side": 1,
"message": "Checked this: the 3 functions that use `max_y \u003d (uint32_t)src_height \u003c\u003c 16` are the box filter functions (`ScalePlaneBox`, `ScalePlaneBox_16`, and `ScaleUVBox`). In those functions, `iy \u003d uy \u003e\u003e 16` is the inclusive start row (computed before `uy +\u003d dy`), and after `uy +\u003d dy` and clamping to `max_y`, `uy \u003e\u003e 16` is the exclusive end row (`boxheight \u003d MIN1((int)(uy \u003e\u003e 16) - iy)`). For the last output row to include the final source row (`src_height - 1`), the exclusive upper bound `uy \u003e\u003e 16` must reach `src_height`, so `max_y` must be `src_height \u003c\u003c 16`. In all other functions (bilinear/simple), `uy` is clamped to `max_y` before extracting `yi \u003d uy \u003e\u003e 16` as a 0-based row index, so the max valid index is `src_height - 1`.",
"parentUuid": "4122bc4c_2330d67a",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "b7ceb797_58aa9f11",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2129,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T22:59:39Z",
"side": 1,
"message": "`FixedDiv(src_height, src_height)` is equal to `1 \u003c\u003c 16`, right?\n\nShould we replace this line with the following?\n\n```\n dy \u003d 1 \u003c\u003c 16; // 1 as 16.16 fixed point\n```\n\nor\n\n```\n dy \u003d 1 \u003c\u003c 16; // FixedDiv(src_height, src_height)\n```",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "28cf3e76_38346912",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2129,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T04:05:30Z",
"side": 1,
"message": "without looking at the code, i recall it implements (x-1) / (y-1). so yes?",
"parentUuid": "b7ceb797_58aa9f11",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "e04ab589_23eed536",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2129,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T05:00:46Z",
"side": 1,
"message": "Yes, `FixedDiv(num, div)` is `(num \u003c\u003c 16) / div` (while `FixedDiv1` is `(num - 1) / (div - 1)`), so `FixedDiv(src_height, src_height)` is indeed `1 \u003c\u003c 16`. Looking closer at this `dst_height \u003d\u003d 1 \u0026\u0026 src_height \u003e\u003d 32768` case (here and in `ScaleSlope`), setting `dy \u003d 1 \u003c\u003c 16` avoids signed 32-bit overflow in `FixedDiv(32768, 1)`, but it causes `y \u003d CENTERSTART(dy, -32768)` to become `0` (row 0) instead of the center of the image (`(src_height \u003c\u003c 15) - 32768`, row 16383.5). Filed b/573815252 to clean this up and fix the `dst \u003d\u003d 1, src \u003d\u003d 32768` center calculation in a follow-up CL.",
"parentUuid": "28cf3e76_38346912",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "ea4edd88_fd437375",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2129,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-15T05:56:13Z",
"side": 1,
"message": "Please add a TODO comment with the bug number `b/573815252`.",
"parentUuid": "e04ab589_23eed536",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "4ac35cbd_d4f16026",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2129,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T06:01:50Z",
"side": 1,
"message": "Done. Added `TODO(b/573815252)` comments here, in `ScalePlane_16`, and in `ScaleSlope`.",
"parentUuid": "ea4edd88_fd437375",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "c8c55c35_2a5db94b",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2248,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T22:59:39Z",
"side": 1,
"message": "Should we replace this line with the following?\n\n```\n dy \u003d 1 \u003c\u003c 16; // 1 as 16.16 fixed point\n```\n\nor\n\n```\n dy \u003d 1 \u003c\u003c 16; // FixedDiv(src_height, src_height)\n```",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "980d4106_7d01077f",
"filename": "source/scale.cc",
"patchSetId": 4
},
"lineNbr": 2248,
"author": {
"id": 1115898
},
"writtenOn": "2026-10-15T04:05:30Z",
"side": 1,
"message": "Acknowledged",
"parentUuid": "c8c55c35_2a5db94b",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "8e6781ed_772860b1",
"filename": "source/scale_argb.cc",
"patchSetId": 4
},
"lineNbr": 909,
"author": {
"id": 1001562
},
"writtenOn": "2026-10-14T23:09:21Z",
"side": 1,
"message": "I verified that the `src_width \u003e 32768` check added here fixed [bug chromium:546841026](https://g-issues.chromium.org/issues/546841026).",
"revId": "8dabf898eeee2e08d41666ee24533da35f4dcee7",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
}
]
}
Reference in New Issue View Git Blame Copy Permalink