Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
478273002:
Use NaN/Inf to represent unresolved/indefinite SMILTimes (Closed)
|
Created:
6 years, 4 months ago by reni Modified:
6 years, 4 months ago Reviewers:
fs CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, rwlbuis, fs, kouhei+svg_chromium.org, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, Steve Block, pdr., Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionUse NaN/Inf to represent unresolved/indefinite SMILTimes.
R=fs@opera.com
BUG=404601
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180823
Patch Set 1 #Patch Set 2 : Updated patch #Patch Set 3 : Fix regression. #
Total comments: 39
Patch Set 4 : Update according to the review. #
Total comments: 3
Patch Set 5 : Updated patch. #
Messages
Total messages: 10 (0 generated)
Unresolved and indefinite values are represented with NaN and infinite values as discussed under https://codereview.chromium.org/406263002/. To remain equivalent with the current behavior, the comparison operators and the usage of std::min/max functions had to be changed. Besides, only finite values were added to containers to avoid problems with Inf/NaN keys.
Sorry for the long delay, vacation time for me =). I'll try to remember to check in on this tomorrow though. Thanks for looking at this! Hopefully there should be some simplifications to reap after this (I added one note about one thing that could be fixed.) Below are some thoughts from my Piña Colada-soaked (I wish...) vacation brain ;-) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.cpp (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.cpp:36: if (a.isUnresolved() || b.isUnresolved()) Note: With this new representation, this should now simplify to just the last line, so that's probably a good follow up to this CL =). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.h (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:39: SMILTime(const SMILTime& o) : m_time(o.m_time) { } While you're here... I believe this could be dropped (it's the same as the default). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:44: SMILTime& operator=(const SMILTime& o) { m_time = o.m_time; return *this; } This should be possible to drop as well. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:47: bool isFinite() const { return m_time < indefiniteValue(); } Use std::isfinite here (hopefully that also dropping some of the other changes, because as is this will return 'false' for the unresolved value (NaN)). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:52: static const double& unresolvedValue() I'd suggest instead just have unresolved()/indefinite() (above) return the std::numeric_limits<...>::foo() directly. I don't think there's a need for these - I'd suspect the std-methods just return the appropriate bit-pattern (should maybe confirm that though...). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:105: return a.isFinite() && a.value() == b.value(); I think the following would work equal well: a.isUnresolved() == b.isUnresolved() || a.value() == b.value() even though it's not equivalent, since 'indefinite' is special-cased (but only in one of the operands...). So, please try the above and see if there's tests complaining... https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:110: return !a.isFinite() || !a.value(); It's _probably_ possible to simplify this (to just the right-hand side), but no rush I suppose. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:119: return !operator==(a, b); This really ought to be possible to have as it previously was. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:130: return a.value() > b.value(); Make this: a.isUnresolved() || a.value() > b.value() (I'm not sure why we need the left-hand side of that though, but we can leave that for later...) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:141: return a.value() < b.value(); Could either use the operator> above (with operands reversed), or be modified in the same way as it. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:152: return a.value() > b.value() || operator==(a, b); This (and <=) could just use the operator> (operator<) from above and drop the initial unresolved-checks. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:495: if (timeList[n].time().isFinite()) Could this be isUnresolved instead? I.e. I don't see why having Inf in the HashSet would be a problem (I can imaging that NaN would be a problem). I also think that 'indefinite' could appear here while 'unresolved' would be less interesting if it did so could be dropped without harm. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:504: else if (value.isFinite() && !existing.contains(value.value())) I think 'indefinite' is [sometimes] valid here, so hopefully doing as prescribed above works. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:807: return duration.isUnresolved() ? SMILTime::indefinite() : duration; I think this could've been left as min(), but maybe better to play it on the safe side [1] =). Another possibility would be to implement a min() with (guaranteed) SMIL-semantics [2]. [1] I.e. hope that the impl. is as prescribed by the spec - !(b < a) ? a : b [2] I believe the main difference is that MIN(indefinite, unresolved) should return indefinite. std::min() should do that too with just the plain values FWIW (but it will return unresolved if the argument order is reversed...) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:812: if (!beginTime.isFinite()) Could we drop this (and the below) back to asserts? It generally seems better for the caller to filter out values. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:882: return std::min(repeatDur, SMILTime::indefinite()); This would probably look more obvious if the min(repeatDur, indefinite) was computed first? SMILTime a = min(repeatDur, ...); SMILTime repeatCountDuration = ...; if (!...) a = min(a, repeatCountDuration); return a; ? https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:906: return resolvedBegin + (preliminaryActiveDuration.isUnresolved() ? preliminaryActiveDuration : std::max(minValue, preliminaryActiveDuration)); Looks like maxValue disappeared? Also, if |preliminaryActiveDuration| is unresolved then the result is undefined too, so that could be filtered earlier (after it's set)?
https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.h (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:105: return a.isFinite() && a.value() == b.value(); On 2014/08/19 21:49:43, fs (ooo) wrote: > I think the following would work equal well: > > a.isUnresolved() == b.isUnresolved() || a.value() == b.value() s/==/&&/
> Below are some thoughts from my Piña Colada-soaked (I wish...) vacation brain ;-) It was tempted to answer your Pina Colada-soaked review with my wine-soaked brain yesterday (it was national holiday here) but I was strong and waited with it until today ;-) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.h (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:39: SMILTime(const SMILTime& o) : m_time(o.m_time) { } On 2014/08/19 21:49:43, fs (ooo) wrote: > While you're here... I believe this could be dropped (it's the same as the > default). Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:44: SMILTime& operator=(const SMILTime& o) { m_time = o.m_time; return *this; } On 2014/08/19 21:49:43, fs (ooo) wrote: > This should be possible to drop as well. Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:47: bool isFinite() const { return m_time < indefiniteValue(); } On 2014/08/19 21:49:43, fs (ooo) wrote: > Use std::isfinite here (hopefully that also dropping some of the other changes, > because as is this will return 'false' for the unresolved value (NaN)). Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:52: static const double& unresolvedValue() On 2014/08/19 21:49:43, fs (ooo) wrote: > I'd suggest instead just have unresolved()/indefinite() (above) return the > std::numeric_limits<...>::foo() directly. I don't think there's a need for these > - I'd suspect the std-methods just return the appropriate bit-pattern (should > maybe confirm that though...). Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:105: return a.isFinite() && a.value() == b.value(); On 2014/08/19 21:49:43, fs (ooo) wrote: > I think the following would work equal well: > > a.isUnresolved() == b.isUnresolved() || a.value() == b.value() > > even though it's not equivalent, since 'indefinite' is special-cased (but only > in one of the operands...). > > So, please try the above and see if there's tests complaining... Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:119: return !operator==(a, b); On 2014/08/19 21:49:43, fs (ooo) wrote: > This really ought to be possible to have as it previously was. Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:130: return a.value() > b.value(); On 2014/08/19 21:49:43, fs (ooo) wrote: > Make this: > > a.isUnresolved() || a.value() > b.value() > > (I'm not sure why we need the left-hand side of that though, but we can leave > that for later...) Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:141: return a.value() < b.value(); On 2014/08/19 21:49:43, fs (ooo) wrote: > Could either use the operator> above (with operands reversed), or be modified in > the same way as it. Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:152: return a.value() > b.value() || operator==(a, b); On 2014/08/19 21:49:43, fs (ooo) wrote: > This (and <=) could just use the operator> (operator<) from above and drop the > initial unresolved-checks. Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:495: if (timeList[n].time().isFinite()) On 2014/08/19 21:49:43, fs (ooo) wrote: > Could this be isUnresolved instead? I.e. I don't see why having Inf in the > HashSet would be a problem (I can imaging that NaN would be a problem). I also > think that 'indefinite' could appear here while 'unresolved' would be less > interesting if it did so could be dropped without harm. Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:504: else if (value.isFinite() && !existing.contains(value.value())) On 2014/08/19 21:49:44, fs (ooo) wrote: > I think 'indefinite' is [sometimes] valid here, so hopefully doing as prescribed > above works. HashSet can really contain Inf value without consequences however looking for an Inf key in it causes an assertion failure in HashTableKeyChecker<...>::checkKey(key). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:807: return duration.isUnresolved() ? SMILTime::indefinite() : duration; On 2014/08/19 21:49:44, fs (ooo) wrote: > I think this could've been left as min(), but maybe better to play it on the > safe side [1] =). > > Another possibility would be to implement a min() with (guaranteed) > SMIL-semantics [2]. > > [1] I.e. hope that the impl. is as prescribed by the spec - !(b < a) ? a : b > [2] I believe the main difference is that MIN(indefinite, unresolved) should > return indefinite. std::min() should do that too with just the plain values FWIW > (but it will return unresolved if the argument order is reversed...) Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:812: if (!beginTime.isFinite()) On 2014/08/19 21:49:43, fs (ooo) wrote: > Could we drop this (and the below) back to asserts? It generally seems better > for the caller to filter out values. I set them back to asserts. However, are we sure that these asserts are needed at all if the caller filtered out the unresolved values already? https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:882: return std::min(repeatDur, SMILTime::indefinite()); On 2014/08/19 21:49:44, fs (ooo) wrote: > This would probably look more obvious if the min(repeatDur, indefinite) was > computed first? > > SMILTime a = min(repeatDur, ...); > SMILTime repeatCountDuration = ...; > if (!...) > a = min(a, repeatCountDuration); > return a; > > ? Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:906: return resolvedBegin + (preliminaryActiveDuration.isUnresolved() ? preliminaryActiveDuration : std::max(minValue, preliminaryActiveDuration)); On 2014/08/19 21:49:44, fs (ooo) wrote: > Looks like maxValue disappeared? Also, if |preliminaryActiveDuration| is > unresolved then the result is undefined too, so that could be filtered earlier > (after it's set)? Done.
>> Below are some thoughts from my Piña Colada-soaked (I wish...) vacation brain ;-) > >It was tempted to answer your Pina Colada-soaked review with my wine-soaked brain yesterday (it was national holiday here) but I was strong and waited with it until today ;-) =) Almost there... just few more tweaks to the SVGSMILElement.cpp changes. (It's also be great if you could add a comment about the expected ordering or different values I made a note about below.) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.h (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:130: return a.value() > b.value(); On 2014/08/19 21:49:43, fs (ooo) wrote: > Make this: > > a.isUnresolved() || a.value() > b.value() > > (I'm not sure why we need the left-hand side of that though, but we can leave > that for later...) It dawned on me why we want the LHS here. In general we want to get an ordering so that: finite < indefinite < unresolved and hence: isfinite(v) < Inf < NaN (compare to previous representation: v < FLT_MAX < DBL_MAX) and the first part is now handled by IEEE754 (i.e. operator< for double), but we need the LHS to guarantee that NaN (unresolved) orders correctly. Maybe that's worth noting in a comment somewhere around here... https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:504: else if (value.isFinite() && !existing.contains(value.value())) On 2014/08/21 14:20:06, reni wrote: > On 2014/08/19 21:49:44, fs (ooo) wrote: > > I think 'indefinite' is [sometimes] valid here, so hopefully doing as > prescribed > > above works. > > HashSet can really contain Inf value without consequences however looking for an > Inf key in it causes an assertion failure in > HashTableKeyChecker<...>::checkKey(key). Ok. I noticed: static T emptyValue() { return std::numeric_limits<T>::infinity(); } (and -Inf is deleted-value) I guess that means that indefinite/Inf needs to be special cased =/ (short of using a tweaked HashSet). https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:812: if (!beginTime.isFinite()) On 2014/08/21 14:20:06, reni wrote: > On 2014/08/19 21:49:43, fs (ooo) wrote: > > Could we drop this (and the below) back to asserts? It generally seems better > > for the caller to filter out values. > > I set them back to asserts. However, are we sure that these asserts are needed > at all if the caller filtered out the unresolved values already? If nothing else, they serve to communicate the preconditions of these methods. I'm not quite sure what the original assert was for, because there would've been asserts in the constructor first. If you want to drop the asserts, feel free. https://codereview.chromium.org/478273002/diff/60001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:897: return SMILTime::unresolved(); Actually, min(maxValue, max(minValue, pAD)); pAD=unresolved => maxValue (max(minValue, unresolved) => unresolved, min(maxValue, unresolved) => maxValue) so the result should either be resolvedBegin + maxValue (if specified and valid) or 'indefinite'. I also think that it should "just work(TM)" with the fully defined SMILTime operators. If not, I suggest just setting pAD to 'indefinite' (if 'unresolved'). https://codereview.chromium.org/478273002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:1287: return; return => continue
https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTime.h (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTime.h:130: return a.value() > b.value(); On 2014/08/21 15:30:39, fs (ooo) wrote: > On 2014/08/19 21:49:43, fs (ooo) wrote: > > Make this: > > > > a.isUnresolved() || a.value() > b.value() > > > > (I'm not sure why we need the left-hand side of that though, but we can leave > > that for later...) > > It dawned on me why we want the LHS here. In general we want to get an ordering > so that: > > finite < indefinite < unresolved > > and hence: > > isfinite(v) < Inf < NaN > > (compare to previous representation: v < FLT_MAX < DBL_MAX) > > and the first part is now handled by IEEE754 (i.e. operator< for double), but we > need the LHS to guarantee that NaN (unresolved) orders correctly. Maybe that's > worth noting in a comment somewhere around here... Done. https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:504: else if (value.isFinite() && !existing.contains(value.value())) On 2014/08/21 15:30:39, fs (ooo) wrote: > On 2014/08/21 14:20:06, reni wrote: > > On 2014/08/19 21:49:44, fs (ooo) wrote: > > > I think 'indefinite' is [sometimes] valid here, so hopefully doing as > > prescribed > > > above works. > > > > HashSet can really contain Inf value without consequences however looking for > an > > Inf key in it causes an assertion failure in > > HashTableKeyChecker<...>::checkKey(key). > > Ok. I noticed: > > static T emptyValue() { return std::numeric_limits<T>::infinity(); } > > (and -Inf is deleted-value) > > I guess that means that indefinite/Inf needs to be special cased =/ (short of > using a tweaked HashSet). I agree with =/, but done :) https://codereview.chromium.org/478273002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:812: if (!beginTime.isFinite()) On 2014/08/21 15:30:39, fs (ooo) wrote: > On 2014/08/21 14:20:06, reni wrote: > > On 2014/08/19 21:49:43, fs (ooo) wrote: > > > Could we drop this (and the below) back to asserts? It generally seems > better > > > for the caller to filter out values. > > > > I set them back to asserts. However, are we sure that these asserts are needed > > at all if the caller filtered out the unresolved values already? > > If nothing else, they serve to communicate the preconditions of these methods. > I'm not quite sure what the original assert was for, because there would've been > asserts in the constructor first. If you want to drop the asserts, feel free. They are dropped. https://codereview.chromium.org/478273002/diff/60001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/478273002/diff/60001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:1287: return; On 2014/08/21 15:30:39, fs (ooo) wrote: > return => continue Done.
LGTM! (Please also take a second [or two] to edit the subject/description - currently the subject will be "Represent SMILTime::indefiniteValue and SMILTime::unresolvedValue values" which is sortof "hanging". Suggestion for new "subject": "Use NaN/Inf to represent unresolved/indefinite SMILTimes")
The CQ bit was checked by rhodovan.u-szeged@partner.samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rhodovan.u-szeged@partner.samsung.com/...
Message was sent while issue was closed.
Committed patchset #5 (80001) as 180823 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
