Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1638323002:
Filx complier issue [-Werror, -Wvla] (Closed)
DescriptionFix complier issue [-Werror, -Wvla]
BUG=skia:4861
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1638323002
Committed: https://skia.googlesource.com/skia/+/24716be1d1e6d7fba6ee8604823ce326fd69be1e
Patch Set 1 #
Total comments: 6
Patch Set 2 : Put stuff on the heap unconditionally #
Total comments: 4
Messages
Total messages: 24 (5 generated)
Description was changed from ========== Filx complier issue [-Werror, -Wvla] BUG=skia: ========== to ========== Filx complier issue [-Werror, -Wvla] BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Filx complier issue [-Werror, -Wvla] BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix complier issue [-Werror, -Wvla] BUG=skia:4861 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
yujieqin@google.com changed reviewers: + benjaminwagner@google.com, msarett@google.com, mtklein@google.com, scroggo@google.com
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. Any reason to not put this on the heap?
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. On 2016/01/27 14:51:05, msarett wrote: > Any reason to not put this on the heap? I do not have preference here. Just follow the code in https://bugs.chromium.org/p/skia/issues/detail?id=4861#c1. Should I do somehow differently?
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. Sorry, that number was a bit of a joke. How long is a typical row? Is it a reasonable thing to put on the stack? If not, SkAutoTMalloc<uint8_t> srcRow(width * 3); will put it on the heap unconditionally as Matt suggests.
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. Normally I would think it should be around thousands of pixels. But it is not possible to limit the width of an image somehow (e.g. panorama image). I think the condition here could still be good?
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. On 2016/01/27 14:57:53, yujieqin wrote: > Normally I would think it should be around thousands of pixels. But it is not > possible to limit the width of an image somehow (e.g. panorama image). I think > the condition here could still be good? Let's use this: SkAutoTMalloc<uint8_t> srcRow(width * 3); It matches our other codecs, it avoid putting a (potentially very large) allocation on the stack, and it prevents us from worrying about how large width can truly be.
https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:409: static const int kStackBytes = 640 * 1024; // Ought to be enough for anybody. On 2016/01/27 15:02:14, msarett wrote: > On 2016/01/27 14:57:53, yujieqin wrote: > > Normally I would think it should be around thousands of pixels. But it is not > > possible to limit the width of an image somehow (e.g. panorama image). I think > > the condition here could still be good? > > Let's use this: > SkAutoTMalloc<uint8_t> srcRow(width * 3); > > It matches our other codecs, it avoid putting a (potentially very large) > allocation on the stack, and it prevents us from worrying about how large width > can truly be. Done.
lgtm
The CQ bit was checked by yujieqin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1638323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1638323002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix complier issue [-Werror, -Wvla] BUG=skia:4861 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix complier issue [-Werror, -Wvla] BUG=skia:4861 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/24716be1d1e6d7fba6ee8604823ce326fd69be1e ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/24716be1d1e6d7fba6ee8604823ce326fd69be1e
Message was sent while issue was closed.
https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); Does this really work?
Message was sent while issue was closed.
https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); On 2016/01/29 19:36:08, Ben Wagner wrote: > Does this really work? Nope, it's quite wrong. If the old VLA code was correct, this should now read width*3.
Message was sent while issue was closed.
https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); On 2016/01/29 19:47:48, mtklein wrote: > On 2016/01/29 19:36:08, Ben Wagner wrote: > > Does this really work? > > Nope, it's quite wrong. If the old VLA code was correct, this should now read > width*3. Agreed. Is someone already fixing this?
Message was sent while issue was closed.
https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); On 2016/01/29 20:44:29, scroggo wrote: > On 2016/01/29 19:47:48, mtklein wrote: > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > Does this really work? > > > > Nope, it's quite wrong. If the old VLA code was correct, this should now read > > width*3. > > Agreed. Is someone already fixing this? I've posted a fix at crrev.com/1643373002. How did it work before though? Gold looks to have some valid results, when it should never be right.
Message was sent while issue was closed.
On 2016/01/29 21:01:30, scroggo wrote: > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp > File src/codec/SkRawCodec.cpp (right): > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... > src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); > On 2016/01/29 20:44:29, scroggo wrote: > > On 2016/01/29 19:47:48, mtklein wrote: > > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > > Does this really work? > > > > > > Nope, it's quite wrong. If the old VLA code was correct, this should now > read > > > width*3. > > > > Agreed. Is someone already fixing this? > > I've posted a fix at crrev.com/1643373002. > > How did it work before though? Gold looks to have some valid results, when it > should never be right. When using a VLA, sizeof() does magically work. uint8_t row[width*3]; .... int x = sizeof(row); gives you x = width*3. It's not, however, legal C++. It is legal C.
Message was sent while issue was closed.
On 2016/01/29 21:04:53, mtklein wrote: > On 2016/01/29 21:01:30, scroggo wrote: > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp > > File src/codec/SkRawCodec.cpp (right): > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... > > src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); > > On 2016/01/29 20:44:29, scroggo wrote: > > > On 2016/01/29 19:47:48, mtklein wrote: > > > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > > > Does this really work? > > > > > > > > Nope, it's quite wrong. If the old VLA code was correct, this should now > > read > > > > width*3. > > > > > > Agreed. Is someone already fixing this? > > > > I've posted a fix at crrev.com/1643373002. > > > > How did it work before though? Gold looks to have some valid results, when it > > should never be right. > > When using a VLA, sizeof() does magically work. > > uint8_t row[width*3]; > .... > int x = sizeof(row); > gives you x = width*3. > > It's not, however, legal C++. It is legal C. But I mean the code that landed two days ago: SkAutoTMalloc<uint8_t> srcRow(width * 3); int x = sizeof(srcRow); Or was this not working and the results we see from gold were from before this?
Message was sent while issue was closed.
On 2016/01/29 21:13:24, scroggo wrote: > On 2016/01/29 21:04:53, mtklein wrote: > > On 2016/01/29 21:01:30, scroggo wrote: > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp > > > File src/codec/SkRawCodec.cpp (right): > > > > > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... > > > src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); > > > On 2016/01/29 20:44:29, scroggo wrote: > > > > On 2016/01/29 19:47:48, mtklein wrote: > > > > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > > > > Does this really work? > > > > > > > > > > Nope, it's quite wrong. If the old VLA code was correct, this should > now > > > read > > > > > width*3. > > > > > > > > Agreed. Is someone already fixing this? > > > > > > I've posted a fix at crrev.com/1643373002. > > > > > > How did it work before though? Gold looks to have some valid results, when > it > > > should never be right. > > > > When using a VLA, sizeof() does magically work. > > > > uint8_t row[width*3]; > > .... > > int x = sizeof(row); > > gives you x = width*3. > > > > It's not, however, legal C++. It is legal C. > > But I mean the code that landed two days ago: > > SkAutoTMalloc<uint8_t> srcRow(width * 3); > int x = sizeof(srcRow); > > Or was this not working and the results we see from gold were from before this? Must be buffer.fRowStep doesn't matter? Seems plausible, as we're working on one row at a time anyway right?
Message was sent while issue was closed.
On 2016/01/29 21:20:05, mtklein wrote: > On 2016/01/29 21:13:24, scroggo wrote: > > On 2016/01/29 21:04:53, mtklein wrote: > > > On 2016/01/29 21:01:30, scroggo wrote: > > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp > > > > File src/codec/SkRawCodec.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... > > > > src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); > > > > On 2016/01/29 20:44:29, scroggo wrote: > > > > > On 2016/01/29 19:47:48, mtklein wrote: > > > > > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > > > > > Does this really work? > > > > > > > > > > > > Nope, it's quite wrong. If the old VLA code was correct, this should > > now > > > > read > > > > > > width*3. > > > > > > > > > > Agreed. Is someone already fixing this? > > > > > > > > I've posted a fix at crrev.com/1643373002. > > > > > > > > How did it work before though? Gold looks to have some valid results, when > > it > > > > should never be right. > > > > > > When using a VLA, sizeof() does magically work. > > > > > > uint8_t row[width*3]; > > > .... > > > int x = sizeof(row); > > > gives you x = width*3. > > > > > > It's not, however, legal C++. It is legal C. > > > > But I mean the code that landed two days ago: > > > > SkAutoTMalloc<uint8_t> srcRow(width * 3); > > int x = sizeof(srcRow); > > > > Or was this not working and the results we see from gold were from before > this? > > Must be buffer.fRowStep doesn't matter? > Seems plausible, as we're working on one row at a time anyway right? Haha, yep, that makes sense.
Message was sent while issue was closed.
On 2016/01/29 21:32:42, scroggo wrote: > On 2016/01/29 21:20:05, mtklein wrote: > > On 2016/01/29 21:13:24, scroggo wrote: > > > On 2016/01/29 21:04:53, mtklein wrote: > > > > On 2016/01/29 21:01:30, scroggo wrote: > > > > > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cpp > > > > > File src/codec/SkRawCodec.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1638323002/diff/20001/src/codec/SkRawCodec.cp... > > > > > src/codec/SkRawCodec.cpp:418: buffer.fRowStep = sizeof(srcRow); > > > > > On 2016/01/29 20:44:29, scroggo wrote: > > > > > > On 2016/01/29 19:47:48, mtklein wrote: > > > > > > > On 2016/01/29 19:36:08, Ben Wagner wrote: > > > > > > > > Does this really work? > > > > > > > > > > > > > > Nope, it's quite wrong. If the old VLA code was correct, this > should > > > now > > > > > read > > > > > > > width*3. > > > > > > > > > > > > Agreed. Is someone already fixing this? > > > > > > > > > > I've posted a fix at crrev.com/1643373002. > > > > > > > > > > How did it work before though? Gold looks to have some valid results, > when > > > it > > > > > should never be right. > > > > > > > > When using a VLA, sizeof() does magically work. > > > > > > > > uint8_t row[width*3]; > > > > .... > > > > int x = sizeof(row); > > > > gives you x = width*3. > > > > > > > > It's not, however, legal C++. It is legal C. > > > > > > But I mean the code that landed two days ago: > > > > > > SkAutoTMalloc<uint8_t> srcRow(width * 3); > > > int x = sizeof(srcRow); > > > > > > Or was this not working and the results we see from gold were from before > > this? > > > > Must be buffer.fRowStep doesn't matter? > > Seems plausible, as we're working on one row at a time anyway right? > > Haha, yep, that makes sense. Sorry for late, I was OoO in Friday. Thanks for the help to fix the issue. :) |
