Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1945283003:
Fix the DCHECK failure in render_text_harfbuzz.cc. (Closed)
DescriptionFix the DCHECK failure in render_text_harfbuzz.cc.
It's reported that DCHECK failed in AddLineSegment(). The reason might be
that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute
error margin to compare two floats since last_segment.x_range.end() and
segment.x_range.start() might be very large and epsilon() could end up being
smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()).
Or there's likely floating math rounding errors somewhere that caused this DCHECK error.
We change the DCHECK to check the difference between one run and the next
is less than 1.0 (i.e., less than one pixel).
BUG=609004
Committed: https://crrev.com/e5a5bbfd2365c4c9f1f6cf97a56f0cd15b1c3132
Cr-Commit-Position: refs/heads/master@{#391724}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address msw@'s comment. #Messages
Total messages: 16 (9 generated)
Description was changed from ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the smallest rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ========== to ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the smallest rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ==========
Description was changed from ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the smallest rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ========== to ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the smallest rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ==========
Description was changed from ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the smallest rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ========== to ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ==========
xdai@chromium.org changed reviewers: + msw@chromium.org
msw@, could you help review this CL please? Thanks!
https://codereview.chromium.org/1945283003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/1945283003/diff/1/ui/gfx/render_text_harfbuzz... ui/gfx/render_text_harfbuzz.cc:409: std::numeric_limits<float>::epsilon()); It seems like there's likely floating math rounding errors somewhere (failing on 3.05176e-05 vs. 1.19209e-07). I think it's still a useful DCHECK if we just change it to check that the difference is less than 1.0f (ie. there's less than a pixel between one run and the next)? They really should be equal, but checking <1.0f is better than nothing at all. WDYT?
Description was changed from ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). It might be not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). The easiest way to fix this is just removing this DCHECK since it might not be worthy to write an additional function to compare these two float numbers. BUG=609004 ========== to ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ==========
Description was changed from ========== Remove the DCHECK in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ========== to ========== Fix the DCHECK failure in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ==========
Addressed! Please take another look, thanks for your review. https://codereview.chromium.org/1945283003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/1945283003/diff/1/ui/gfx/render_text_harfbuzz... ui/gfx/render_text_harfbuzz.cc:409: std::numeric_limits<float>::epsilon()); On 2016/05/04 22:03:59, msw wrote: > It seems like there's likely floating math rounding errors somewhere (failing on > 3.05176e-05 vs. 1.19209e-07). I think it's still a useful DCHECK if we just > change it to check that the difference is less than 1.0f (ie. there's less than > a pixel between one run and the next)? They really should be equal, but checking > <1.0f is better than nothing at all. WDYT? I agree with you. Thanks for your suggestion!
lgtm
The CQ bit was checked by xdai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945283003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945283003/20001
Message was sent while issue was closed.
Description was changed from ========== Fix the DCHECK failure in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ========== to ========== Fix the DCHECK failure in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix the DCHECK failure in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 ========== to ========== Fix the DCHECK failure in render_text_harfbuzz.cc. It's reported that DCHECK failed in AddLineSegment(). The reason might be that it's not accurate to use std::numeric_limits<float>::epsilon() as the absolute error margin to compare two floats since last_segment.x_range.end() and segment.x_range.start() might be very large and epsilon() could end up being smaller than the rounding error of std::abs(last_segment.x_range.end() - segment.x_range.start()). Or there's likely floating math rounding errors somewhere that caused this DCHECK error. We change the DCHECK to check the difference between one run and the next is less than 1.0 (i.e., less than one pixel). BUG=609004 Committed: https://crrev.com/e5a5bbfd2365c4c9f1f6cf97a56f0cd15b1c3132 Cr-Commit-Position: refs/heads/master@{#391724} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e5a5bbfd2365c4c9f1f6cf97a56f0cd15b1c3132 Cr-Commit-Position: refs/heads/master@{#391724} |
