<div dir="ltr">Jim,<br><br><div><div class="gmail_extra"><br><div class="gmail_quote">2015-08-25 0:08 GMT+02:00 Jim Graham <span dir="ltr"><<a href="mailto:james.graham@oracle.com" target="_blank">james.graham@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Laurent,<br>
<br>
I'm feeling much better this week (and can even start typing with my bad hand in short spurts now), so I'll hopefully be more on the ball now.<br></blockquote><div><br></div><div>Great ! Hope your hand is not too bad.<br></div><div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Short version - looks good to go, consider the following input at your own discretion and go ahead and push.<br></blockquote><div><br></div><div>Excellent. <br></div><div><br></div><div>Here are my comments:<br></div><div>Â </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewing s3.4:<span><br>
<br>
On 8/17/15 2:34 PM, Laurent Bourgès wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Changes:<br>
- Renderer: remove unused D_ERR_STEP_MAX and 0x1 replaced by 1 for<br>
readability<br>
</blockquote>
<br></span>
Renderer, line 319 - it makes sense to me that the scale changes would be based off of the same metric. Basically the constraint is keeping the speed (dxy) between INC and DEC bounds. I'll admit, though, that I haven't been through the actual derivations of the math here, but I thought I'd give my 2 cents on your comment there.<br></blockquote><div><br></div><div>I left a comment as changing the metric will require adjusting the bounds + tests (postponed later).<br><br>According to my understandings, the error between the linear segment and the curve is <= 8 x | acceleration(t) | (Graphics Gem I) so I would prefer using the ddx|y. Besides, I would use th squared norm = ddx*ddx + ddy*ddy instead of abs(ddx).<br>I think the speed dx|y describes the curve slope so it does not correspond to the approximation error, but it seems you already use such constraints in another renderer (ProcessPath.c).<br><br></div><div>Another point: filled curves (handled by Renderer directly) are not monotonic so this error approximation is then not valid ! <br>Maybe we should split the curve as the Stroker does (at cups, inflexion points ...)<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Renderer, line 479 - I often put extra parens in for cases like this because it isn't common knowledge whether cast or add is higher in precedence. I generally assume no better knowledge than the basic PEMDAS rules that we learn in grade school. But that's just my style (I also don't have the OOO table memorized, but maybe that's just my senility ;).<br></blockquote><div><br>Done. <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
0x1 vs 1 - I usually use 0x1 when I'm using numbers for their bit patterns, so | and & typically use 0x constants in my style philosophy. In the case of "1" it's a degenerate case that doesn't seem to matter, but the added 0x tells me to think in terms of bits, not cardinal values.<br></blockquote><div><br></div><div>Fixed<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Renderer, note for future investigation - we store the direction flag in Y_MAX, but then we need to do manipulation to process Y_MAX and we also need more manipulation to transfer that bit to crossing values. What if instead we stored it in the lowest bit of curx? We'd then have 31.31 fixed point, we'd have to process slope so that its whole part was doubled (to skip affecting the LSbit of curx) and we'd have to do the carry using "((error >> 31) << 1)", but we'd get the transfer of the direction bit to the crossing for free and all in all, the total operations may be reduced (at the cost of 1 bit of X coordinate range).<span><br></span></blockquote><div><br></div><div>I seems a very good idea to evaluate soon: it will save several shift operations (+ 1 Unsafe access). <br>I would also make error, bump_x, bump_error as 31.31 FP (LSB=0) so it would be consistent and may simplify even more computations.<br></div><div> <br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hope it is good now, I plan to work soon on the Array Cache & dispose code.<br>
</blockquote>
<br></span>
Looks great! Consider the style issues I mentioned above if you wish and otherwise it is good to go. I don't need to see any further webrevs on those issues.<br></blockquote><div><br></div><div>Thanks. I started working on RLE / pixel coverage encoding but still expect to improve Array Cache next.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
One more suggestion for optimization below can be considered in subsequent work:<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  For CHECK_NAN, how fast is Math.isNaN? Is it worth first comparing<br>
  the intpart to the value that NaN converts to before calling isNaN?<br>
  The majority case, by a wide margin, is that the value is not NaN.<br>
<br>
<br>
That's the aim: I test first the most probable case (a <= intpart) for<br>
ceil() and then check possible overflow and finally check for NaN values<br>
to avoid adding / substracting 1 !<br>
I made benchmarks and this implementation optimized for the integer<br>
domain works very well: 2x faster.<br>
</blockquote>
<br></span>
I guess I wasn't clear. I understand why the 3 parts of the test are in the order that they are in, but I was referring only to the "CHECK_NAN && Float.isNaN(a)" clause. Is that worth adding one more test for "CHECK_NAN && intpart == NAN_TO_INT_VAL && Float.isNaN(a)" in that one clause?<br>
<br>
For the ceil_int case, a very common case is that a is non-negative and non-integer. In that case, "a <= intpart" will fail and you will do the CHECK_NAN test before you can return "intpart+1".<br>
<br>
For the floor_int case, you would only get to the CHECK_NAN case on negative-non-integers which we don't expect a lot of, but in general, the added test might make it slightly faster.<span><br></span></blockquote><div><br></div><div>I quickly tried but sorry it provides no gains: Float.isNaN() is just implemented as (a != a) so it is faster with only 1 condition (a != a) than 2 (intval == 0 && (a != a)).</div><div>Â <br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  But the more important question may be around how much benefit these<br>
  bring compared to the additional testing overhead of so many<br>
  combinations ?<br>
<br>
<br>
I advocate it is not easy to test all combinations but it seems not a<br>
reasonable testing objective.<br>
<br>
As I said, it is mainly tuning or quality settings but only few are<br>
important (subpixels X / Y, tile size, initial pixel size).<br>
</blockquote>
<br></span>
Another thing to consider is that customers would have to do lots of testing of performance of tweaking these values and so they would discover the problems and report them to us before they become reliant on them. However, down the road they may accept version+1 of Marlin into their existing environment without fully testing and we may introduce a bug that might take them by surprise.<br></blockquote><div><br></div><div>Let's discuss / review that production aspect when Marlin will pass tests & benchmarks. I think several tuning settings may become useless at that time.<br><br></div><div>Laurent<br></div></div></div></div></div>