Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728633002:
Add utility set classes to support getUserMedia constraint proccessing. (Closed)
|
Created:
3 years, 9 months ago by Guido Urdaneta Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, jam, avayvod+watch_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, jasonroberts+watch_google.com, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd utility set classes to support getUserMedia constraint proccessing.
See https://w3c.github.io/mediacapture-main/#constrainable-interface
This CL will be followed by CLs implementing algorithms that use the
classes provided by this CL.
BUG=657733
Review-Url: https://codereview.chromium.org/2728633002
Cr-Commit-Position: refs/heads/master@{#457137}
Committed: https://chromium.googlesource.com/chromium/src/+/ebf283a6e622fdd02d0ab9ee2386dfa50ebcc4e3
Patch Set 1 #
Total comments: 78
Patch Set 2 : address comments by reviewers #Patch Set 3 : Add missing#include and make ResolutionSet::ClosestPointTo private #
Total comments: 32
Patch Set 4 : Address hbos comments #Patch Set 5 : fix Windows compile issues + comments/formatting #
Dependent Patchsets: Messages
Total messages: 37 (20 generated)
Description was changed from ========== Add utility set classes to support getUserMedia constraint proccessing. See https://w3c.github.io/mediacapture-main/#constrainable-interface This CL will be followed by CLs implementing algorithms that use the classes provided by this CL. BUG=657733 ========== to ========== Add utility set classes to support getUserMedia constraint proccessing. See https://w3c.github.io/mediacapture-main/#constrainable-interface This CL will be followed by CLs implementing algorithms that use the classes provided by this CL. BUG=657733 ==========
guidou@chromium.org changed reviewers: + hbos@chromium.org, hta@chromium.org
Hi, PTAL.
The kTolerance thing is the only place I found that looks like it should lead to a functional code change. The other comments are for clarity. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:33: std::fabs((d1 - d2) / d2) <= kTolerance); I'm trying to imagine a realistic case where you would want the first case to trigger that isn't also triggered by the second case. if d2 = 1e-5 and d1 = 1.1e-5, the first case will trigger (difference is 1e-6), but the second case will not (1e-6 / 1e-5 = 0.1, which is >> kTolerance). Is this a desired result? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:52: int DimensionFromConstraintValue(long dimension) { Why return value int when argument is long? Consistency seems like a Good Thing, even though it's not going to matter. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:78: ? 0.0 Why 0.0 and not ResolutionSet::kMinConstrainedAspectRatio? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:98: int ToValidConstrainedDimension(long dimension) { When you have these functions that change the value rather than convert the type, I think you should call them ClampToValidConstrainedDimension and equivalents. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:462: default: This is the case that will handle 2 and 3 ideal values. I think you're making the assumption that we can reduce this to the 3-value case by calculating the aspect ratio from width and height and vice versa - if so, make this explicit by adding a comment, making the case branch case 2: case 3: and adding a default branch with a NOTREACHED(). https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.h (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:67: // by range such as strings, booleans and certain constraints of type long. Better language: "... not suitable to be constrained by a range. Examples are strings..." https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:68: // The set of elements of an unconstrained set is application defined. I don't understand what this sentence tries to say. Delete? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:83: DiscreteSet(const DiscreteSet& other) = default; C++ query: Is there any code-generation effect from declaring all these defaults? I thought they'd be declared by default as long as there's no restricted members in the class. If the point is to make it explicit that the class has them, that's a valid reason, but it looks odd to me. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:107: // Both sets are finite and nonempty. s/finite/constrained/ https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:130: std::vector<T> elements_; Query: Would this work with std::set<>? In that case, the only difference between DiscreteSet() and set<> seems to be the FirstElement operator and the ability to have "anything goes" sets. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:139: media::limits::kMaxDimension - 1; kMaxDimension is already 32767 - I think you're using kMaxConstrainedDimension+1 as a flag value saying "very high" - if so, add a comment here saying that - or better, make a new constant with that value and a descriptive name, such as "kNotReallyConstrainedOnMaxDimension" (that's a bad name!!!) https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:148: class Point { I'm not sure of the value of another point class. We already have, for instance, gfx::Point with many of the same properties. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:176: // Returns the square Euclidean distance from |p1| to |p2|. Does it return the square of the distance or the distance? If the latter, remove "square". https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:305: // Utilities that return ResolutionCandidateSets constrained on a specific They return |ResolutionSet|s, not ResolutionCandidateSets. Leftover from rename? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:68: // functions that use iterative numeric methods, it is less accurate in practice Is this ("iterative numeric methods") a reference to atan(), or the std::<geometry> functions in general? Does the difference matter to the tests? Suggest "relies on library functions, which may be less accurate in practice than.... this means that results have to be compared using ApproximatelyEquals". https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:89: } // namespace Why not let the anonymous namespace enclose the tests? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:97: TEST_F(MediaStreamConstraintsUtilSetsTest, VertexListValidity) { This actually tests a test harness function. Add a comment about that.
First round. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:31: return d1 == d2 || Isn't any value minus itself 0.0 even with finite precision? In which case this is already covered by the above if. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:121: Point GetClosestPoint(const std::vector<Point> vertices, const Point& point) { 1-2 vertices: is this to avoid having to check if the point is inside a polygon? From the name and parameters I would expect it to work for polygons. Can you rename this to GetClosestPointOnPointOrLine or similar? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:228: DCHECK_LE(max_width_, kMaxConstrainedDimension + 1); Here and elsewhere I would prefer kMaxDimension. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:313: double ratio = point.width() / point.height(); Watch out for divide by zero! https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:314: // (0.0, 0.0) is always included in the aspect-ratio range. Why? If min resolution is > (0,0) then it isn't contained? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:319: (point.width() == 0.0 && point.height() == 0.0)); (Good that the comparators take precision errors into account! Otherwise a point computed by ClosestPointTo would likely yield points !ContainsPoint(p).) https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:328: std::min(max_aspect_ratio_, other.max_aspect_ratio_)); I think this is incorrect if there is an offset between resolution sets (different minimum resolutions). With offset polygons the intersection between the two polygons could result in a polygon that starts at the intersection point between two line segments. Enjoy some ASCI art for the intersection between A and B resulting in C. BBBBB B B AAAAAAA.AAAA.AAAAAAAAAA A B AA.AAA A B AAA B A AAAA.AA B AAAA B BBB B BBBBBB BBB ..... . . .......CCCCCC.......... . C CCC... . C CCC . . ....CCC . .... . ... . ...... ... But maybe I'm confused about the vertex sets. I'll ask you in person when you're in. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.h (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:68: // The set of elements of an unconstrained set is application defined. On 2017/03/03 11:02:36, hta - Chromium wrote: > I don't understand what this sentence tries to say. Delete? The unconstrained set is a set that conceptually contains all elements in the universe (https://en.wikipedia.org/wiki/Universe_(mathematics)), represented by a bool instead of filling up the vector with all elements. Would it be more appropriate to call it "the universal set"? Replace the last sentence by a comment explaining that the unconstrained/universal set represents a set containing any possible element. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:73: DiscreteSet() : is_unconstrained_(true) {} I would prefer a static function similar to EmptySet() for the unconstrained/universal set. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:116: return DiscreteSet(std::move(intersection)); std::set would yield O(n log n) instead of O(n^2) but that would only matter for larger sets. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:120: // tie-breaker rule. Add a comment saying that this is not applicable to the unconstrained/universal set. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:130: std::vector<T> elements_; On 2017/03/03 11:02:36, hta - Chromium wrote: > Query: Would this work with std::set<>? > > In that case, the only difference between DiscreteSet() and set<> seems to be > the FirstElement operator and the ability to have "anything goes" sets. (In that case this could be represented by an Optional<std::set<T>> and an Intersection function, but I think a class yields clearer code than "has value".) https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:145: static_cast<double>(kMinConstrainedDimension); Doesn't it make more sense to use max and min (not -1)? Is it important to be able to say that if you constrain it to the max value it's not constrained because it is the maximum? I think we should cover the full range of possible values. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:148: class Point { On 2017/03/03 11:02:36, hta - Chromium wrote: > I'm not sure of the value of another point class. > We already have, for instance, gfx::Point with many of the same properties. Point uses int. There is gfx::PointF and gfx::Vector2dF both using float (not double). The vector would be appropriate considering the vector operations. Prefer gfx::Vector2dF over this. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:159: double AspectRatio() const { return width_ / height_; } Watch out for divide by zero? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:176: // Returns the square Euclidean distance from |p1| to |p2|. On 2017/03/03 11:02:36, hta - Chromium wrote: > Does it return the square of the distance or the distance? > If the latter, remove "square". It's the square, so the function should be renamed DistanceToPointSquared. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:187: double width_; The point has no width and height, only coordinates, x and y are better names for that I think. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:257: // the point closest to (|ideal_height|, |ideal_width|) is returned. It doesn't matter but it surprised/confused me at first: Why heigh-width Cartesian space and not width-height? If point (width,height) is replaced by vector (x,y) then it should be width-height aka x-y space instead of y-x or else it would be confusing.
https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:31: return d1 == d2 || On 2017/03/03 16:01:42, hbos_chromium wrote: > Isn't any value minus itself 0.0 even with finite precision? In which case this > is already covered by the above if. No, it isn't in floating point. Inf-Inf is NaN. And Inf is a common value for aspect ratio. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:33: std::fabs((d1 - d2) / d2) <= kTolerance); On 2017/03/03 11:02:36, hta - Chromium wrote: > I'm trying to imagine a realistic case where you would want the first case to > trigger that isn't also triggered by the second case. > > if d2 = 1e-5 and d1 = 1.1e-5, the first case will trigger (difference is 1e-6), > but the second case will not (1e-6 / 1e-5 = 0.1, which is >> kTolerance). > > Is this a desired result? d1=Inf, d2=Inf. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:52: int DimensionFromConstraintValue(long dimension) { On 2017/03/03 11:02:36, hta - Chromium wrote: > Why return value int when argument is long? Consistency seems like a Good Thing, > even though it's not going to matter. This is intended to transform values coming from blink::LongConstraint (which is long) to int, which is what this class and gfx::Size uses. We could use long instead of int in ResolutionSet, but we would still need a function to sanitize potential bad input from LongConstraint (e.g., negative values). https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:78: ? 0.0 On 2017/03/03 11:02:36, hta - Chromium wrote: > Why 0.0 and not ResolutionSet::kMinConstrainedAspectRatio? The question became obsolete now that the concept of "constrained" was removed from the class, following your other advice. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:98: int ToValidConstrainedDimension(long dimension) { On 2017/03/03 11:02:35, hta - Chromium wrote: > When you have these functions that change the value rather than convert the > type, I think you should call them ClampToValidConstrainedDimension and > equivalents. Acknowledged. I removed the would-be Clamp functions following your other advice. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:121: Point GetClosestPoint(const std::vector<Point> vertices, const Point& point) { On 2017/03/03 16:01:41, hbos_chromium wrote: > 1-2 vertices: is this to avoid having to check if the point is inside a polygon? > From the name and parameters I would expect it to work for polygons. Can you > rename this to GetClosestPointOnPointOrLine or similar? Done. Also expanded the documentation a bit. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:228: DCHECK_LE(max_width_, kMaxConstrainedDimension + 1); On 2017/03/03 16:01:42, hbos_chromium wrote: > Here and elsewhere I would prefer kMaxDimension. Done. Question: Do you refer to a kMaxDimension internal to ResolutionSet (which I have newly introduced with that name using maxInt as value) or to media::limits::kMaxDimension?. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:313: double ratio = point.width() / point.height(); On 2017/03/03 16:01:42, hbos_chromium wrote: > Watch out for divide by zero! Inf aspect ratio is fine. NaN (0/0) or (Inf/Inf) is of more concern, but it is handled correctly. Added some tests with extreme ideal values to more explicitly guard against this kind of issue. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:314: // (0.0, 0.0) is always included in the aspect-ratio range. On 2017/03/03 16:01:42, hbos_chromium wrote: > Why? If min resolution is > (0,0) then it isn't contained? In that case (0,0) would be part of the set of points defined aspect-ratio range, but not part of the ResolutionSet, which is the intersection of the former with the sets defined by min-max height and min-max width. Changed the location of the comment to make it more clear that it refers to the aspect ratio only (note that this is a way to handle 0/0). https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:319: (point.width() == 0.0 && point.height() == 0.0)); On 2017/03/03 16:01:42, hbos_chromium wrote: > (Good that the comparators take precision errors into account! Otherwise a point > computed by ClosestPointTo would likely yield points !ContainsPoint(p).) Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:328: std::min(max_aspect_ratio_, other.max_aspect_ratio_)); On 2017/03/03 16:01:41, hbos_chromium wrote: > I think this is incorrect if there is an offset between resolution sets > (different minimum resolutions). > > With offset polygons the intersection between the two polygons could result in a > polygon that starts at the intersection point between two line segments. > > Enjoy some ASCI art for the intersection between A and B resulting in C. > > BBBBB > B B > AAAAAAA.AAAA.AAAAAAAAAA > A B AA.AAA > A B AAA B > A AAAA.AA B > AAAA B BBB > B BBBBBB > BBB > > ..... > . . > .......CCCCCC.......... > . C CCC... > . C CCC . > . ....CCC . > .... . ... > . ...... > ... > > But maybe I'm confused about the vertex sets. I'll ask you in person when you're > in. The case you are showing is impossible here. All aspect-ratio lines intersect at (0,0), regardless of what the min/max width and height are, so there is no offset. Your concern would apply if we were dealing with more general polygons. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:462: default: On 2017/03/03 11:02:36, hta - Chromium wrote: > This is the case that will handle 2 and 3 ideal values. > > I think you're making the assumption that we can reduce this to the 3-value case > by calculating the aspect ratio from width and height and vice versa - if so, > make this explicit by adding a comment, making the case branch > > case 2: > case 3: > > and adding a default branch with a NOTREACHED(). Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.h (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:67: // by range such as strings, booleans and certain constraints of type long. On 2017/03/03 11:02:36, hta - Chromium wrote: > Better language: "... not suitable to be constrained by a range. Examples are > strings..." Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:68: // The set of elements of an unconstrained set is application defined. On 2017/03/03 16:01:42, hbos_chromium wrote: > On 2017/03/03 11:02:36, hta - Chromium wrote: > > I don't understand what this sentence tries to say. Delete? > > The unconstrained set is a set that conceptually contains all elements in the > universe (https://en.wikipedia.org/wiki/Universe_(mathematics)), represented by > a bool instead of filling up the vector with all elements. Would it be more > appropriate to call it "the universal set"? > > Replace the last sentence by a comment explaining that the > unconstrained/universal set represents a set containing any possible element. Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:68: // The set of elements of an unconstrained set is application defined. On 2017/03/03 11:02:36, hta - Chromium wrote: > I don't understand what this sentence tries to say. Delete? Addressed following hbos' suggestion. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:73: DiscreteSet() : is_unconstrained_(true) {} On 2017/03/03 16:01:42, hbos_chromium wrote: > I would prefer a static function similar to EmptySet() for the > unconstrained/universal set. Done. Also renamed is_unconstrained to is_universal. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:83: DiscreteSet(const DiscreteSet& other) = default; On 2017/03/03 11:02:36, hta - Chromium wrote: > C++ query: Is there any code-generation effect from declaring all these > defaults? I thought they'd be declared by default as long as there's no > restricted members in the class. > > If the point is to make it explicit that the class has them, that's a valid > reason, but it looks odd to me. This is intended. I want the class to be copyable and movable. If nothing is declared, only copy constructors are generated. If only the move constructors are declared, then it's move only. So all the defaults are required. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:107: // Both sets are finite and nonempty. On 2017/03/03 11:02:36, hta - Chromium wrote: > s/finite/constrained/ Done. Changed the wording to both sets have explicit elements. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:116: return DiscreteSet(std::move(intersection)); On 2017/03/03 16:01:42, hbos_chromium wrote: > std::set would yield O(n log n) instead of O(n^2) but that would only matter for > larger sets. Yes. For small sets vector is faster and makes it easier to designate the first element as "default". If it becomes a problem, we can change it. I expect these sets to have few elements, with 0 and 1 being common sizes. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:120: // tie-breaker rule. On 2017/03/03 16:01:42, hbos_chromium wrote: > Add a comment saying that this is not applicable to the unconstrained/universal > set. Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:130: std::vector<T> elements_; On 2017/03/03 16:01:42, hbos_chromium wrote: > On 2017/03/03 11:02:36, hta - Chromium wrote: > > Query: Would this work with std::set<>? > > > > In that case, the only difference between DiscreteSet() and set<> seems to be > > the FirstElement operator and the ability to have "anything goes" sets. > > (In that case this could be represented by an Optional<std::set<T>> and an > Intersection function, but I think a class yields clearer code than "has > value".) Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:130: std::vector<T> elements_; On 2017/03/03 11:02:36, hta - Chromium wrote: > Query: Would this work with std::set<>? > > In that case, the only difference between DiscreteSet() and set<> seems to be > the FirstElement operator and the ability to have "anything goes" sets. This type is basically a wrapper over std::vector to have a convenient API to do intersection, get a default value (first element) and check if it's empty or unconstrained/universal. When I upload the algorithm for video content the reason for the API will be clearer, but it goes roughly like this: Set set(FromConstraints(constraints.basic().someConstraint)) if (set.IsEmpty()) Overconstrained(); for (auto a : constraints.advanced()) { Set intersection = set.Intersection(FromConstraints(a)); if (!intersection.IsEmpty()) { set = std::move(intersection); } } if (basic.hasIdeal() && set.Contains(ideal)) return ideal; if (set.is_universal()) return kDefaultValue; return set.FirstValue(); set<> does not give us that nice API to express SelectSettings so directly. We might also need a Subset or Superset function to deal with device capabilities, but the current version is good enough to start the review. Also, if we decide to implement more complex discrete sets (e.g., migrate the algorithm for device sources to this), we can redefine GetFirstElement to implement any tie-breaker we want, or we can define additional methods if needed. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:139: media::limits::kMaxDimension - 1; On 2017/03/03 11:02:36, hta - Chromium wrote: > kMaxDimension is already 32767 - I think you're using kMaxConstrainedDimension+1 > as a flag value saying "very high" - if so, add a comment here saying that - or > better, make a new constant with that value and a descriptive name, such as > "kNotReallyConstrainedOnMaxDimension" (that's a bad name!!!) Got rid of all the "Constrained" constants and methods. Sets now support aspect ratio from 0 to Inf, and dimensions from 0 to MaxInt. Clamping values to actual supported resolutions will be the responsibility of the SelectSettings implementations. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:145: static_cast<double>(kMinConstrainedDimension); On 2017/03/03 16:01:42, hbos_chromium wrote: > Doesn't it make more sense to use max and min (not -1)? Is it important to be > able to say that if you constrain it to the max value it's not constrained > because it is the maximum? I think we should cover the full range of possible > values. I think you're right. The class now just supports the full range and there is no concept of "constrained" anymore. It's just a set. No special universal or unconstrained set is needed either, since the set of supported values is determined by the ranges, just like NumericRangeSet. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:148: class Point { On 2017/03/03 11:02:36, hta - Chromium wrote: > I'm not sure of the value of another point class. > We already have, for instance, gfx::Point with many of the same properties. See reply to hbos. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:148: class Point { On 2017/03/03 16:01:42, hbos_chromium wrote: > On 2017/03/03 11:02:36, hta - Chromium wrote: > > I'm not sure of the value of another point class. > > We already have, for instance, gfx::Point with many of the same properties. > > Point uses int. There is gfx::PointF and gfx::Vector2dF both using float (not > double). The vector would be appropriate considering the vector operations. > Prefer gfx::Vector2dF over this. I prefer to keep this class (at least for one more iteration) because it's a pair of doubles. gfx::Vector2dF would be a reasonable replacement, but it uses float. Single precision requires a lot more care with the math due to the low precision. Since we generate just a few values, there is no significant performance or space penalty for using doubles. Note that I reimplemented using gfx::Vector2dF and using a tolerance of 1e-4 one test fails. A second test starts failing at 1e-8. With doubles this happens at 1e-8 and 1e-16, respectively, which means double provides much more stable results. Although the tests include plenty of comparisons with computed values that have errors, it is far from comprehensive in that regard. If we have tests that fail with 1e-4, it would be prudent to use 0.01 or even 0.1 in production (which is kind of large), or spend more time thinking about floating point. The only thing Point "duplicates" with respect to Vector2dF is +,-,==,!= and Dot, which are all one-liners (we can get rid of !=, which is used only in testing). I think reducing our floating-point worries is worth duplicating four one-liners. WDYT? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:159: double AspectRatio() const { return width_ / height_; } On 2017/03/03 16:01:42, hbos_chromium wrote: > Watch out for divide by zero? divide by zero is fine in most cases. NaN (0/0 or Inf/Inf) is a greater concern, but the code handles it OK. There is a new test with extreme ideal values that exercises this kind of thing to a greater extent. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:176: // Returns the square Euclidean distance from |p1| to |p2|. On 2017/03/03 11:02:36, hta - Chromium wrote: > Does it return the square of the distance or the distance? > If the latter, remove "square". Renamed. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:176: // Returns the square Euclidean distance from |p1| to |p2|. On 2017/03/03 16:01:42, hbos_chromium wrote: > On 2017/03/03 11:02:36, hta - Chromium wrote: > > Does it return the square of the distance or the distance? > > If the latter, remove "square". > > It's the square, so the function should be renamed DistanceToPointSquared. Square Euclidean is still a distance :) But renamed anyway to SquareEuclideanDistance. DistanceToPoint was a bad name anyway (it was originally a nonstatic method) https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:187: double width_; On 2017/03/03 16:01:42, hbos_chromium wrote: > The point has no width and height, only coordinates, x and y are better names > for that I think. I like height and width better to avoid confusion with aspect ratios and other code where it's more convenient to think of the points as screen resolutions. Although the Point class has generic 2D-vector operations, that's just a byproduct of the approach chosen to solve the problem of distance to ideal. If we were to change the algorithm, the Point class might not need to be so much like a vector, but it would still have width, height and aspect ratio. Perhaps we should hide the vector-like operations in the .cc file, even at the cost of direct testability. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:257: // the point closest to (|ideal_height|, |ideal_width|) is returned. On 2017/03/03 16:01:42, hbos_chromium wrote: > It doesn't matter but it surprised/confused me at first: Why heigh-width > Cartesian space and not width-height? > If point (width,height) is replaced by vector (x,y) then it should be > width-height aka x-y space instead of y-x or else it would be confusing. Because it is easier to think of aspectRatio as a line of the form width = aspectRatio * height, and have the min aspectRatio be the line below and the max aspectRatio the line above in the Cartesian plane. The difficulty of thinking of aspect ratio as the inverse of the slope of the lines that make up the polygon is a lot worse than having height on the X axis. Also, although having width on the X axis sounds natural because that's how it is on a screen, such a Cartesian plane would not really mimic a screen, but a set of screen sizes. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:305: // Utilities that return ResolutionCandidateSets constrained on a specific On 2017/03/03 11:02:36, hta - Chromium wrote: > They return |ResolutionSet|s, not ResolutionCandidateSets. Leftover from rename? Indeed a leftover. Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:68: // functions that use iterative numeric methods, it is less accurate in practice On 2017/03/03 11:02:36, hta - Chromium wrote: > Is this ("iterative numeric methods") a reference to atan(), or the > std::<geometry> functions in general? Does the difference matter to the tests? > > Suggest "relies on library functions, which may be less accurate in practice > than.... this means that results have to be compared using ApproximatelyEquals". Done. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:89: } // namespace On 2017/03/03 11:02:36, hta - Chromium wrote: > Why not let the anonymous namespace enclose the tests? One of the tests is friended by MediaStreamConstraintsUtilSets, so it cannot be in the anonymous namespace. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:97: TEST_F(MediaStreamConstraintsUtilSetsTest, VertexListValidity) { On 2017/03/03 11:02:36, hta - Chromium wrote: > This actually tests a test harness function. Add a comment about that. Done.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:33: std::fabs((d1 - d2) / d2) <= kTolerance); The relative comparisons are for large values.
lgtm https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:45: // Single-segment, single point or single segment are trivial cases. Nit: remove repeated "single-segment"
The algorithm looks sound to me after close inspection, no changes required. I'll look at the unittest first thing tomorrow. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:31: return d1 == d2 || On 2017/03/06 11:08:22, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > Isn't any value minus itself 0.0 even with finite precision? In which case > this > > is already covered by the above if. > > No, it isn't in floating point. Inf-Inf is NaN. And Inf is a common value for > aspect ratio. Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:228: DCHECK_LE(max_width_, kMaxConstrainedDimension + 1); On 2017/03/06 11:08:22, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > Here and elsewhere I would prefer kMaxDimension. > > Done. > Question: Do you refer to a kMaxDimension internal to ResolutionSet (which I > have newly introduced with that name using maxInt as value) or to > media::limits::kMaxDimension?. This is good. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:313: double ratio = point.width() / point.height(); On 2017/03/06 11:08:22, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > Watch out for divide by zero! > > Inf aspect ratio is fine. NaN (0/0) or (Inf/Inf) is of more concern, but it is > handled correctly. > Added some tests with extreme ideal values to more explicitly guard against this > kind of issue. Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:314: // (0.0, 0.0) is always included in the aspect-ratio range. On 2017/03/06 11:08:22, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > Why? If min resolution is > (0,0) then it isn't contained? > In that case (0,0) would be part of the set of points defined aspect-ratio > range, but not part of the ResolutionSet, which is the intersection of the > former with the sets defined by min-max height and min-max width. > Changed the location of the comment to make it more clear that it refers to the > aspect ratio only (note that this is a way to handle 0/0). Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.cc:328: std::min(max_aspect_ratio_, other.max_aspect_ratio_)); On 2017/03/06 11:08:22, Guido Urdaneta wrote: > On 2017/03/03 16:01:41, hbos_chromium wrote: > > I think this is incorrect if there is an offset between resolution sets > > (different minimum resolutions). > > > > With offset polygons the intersection between the two polygons could result in > a > > polygon that starts at the intersection point between two line segments. > > > > Enjoy some ASCI art for the intersection between A and B resulting in C. > > > > BBBBB > > B B > > AAAAAAA.AAAA.AAAAAAAAAA > > A B AA.AAA > > A B AAA B > > A AAAA.AA B > > AAAA B BBB > > B BBBBBB > > BBB > > > > ..... > > . . > > .......CCCCCC.......... > > . C CCC... > > . C CCC . > > . ....CCC . > > .... . ... > > . ...... > > ... > > > > But maybe I'm confused about the vertex sets. I'll ask you in person when > you're > > in. > > The case you are showing is impossible here. All aspect-ratio lines intersect at > (0,0), regardless of what the min/max width and height are, so there is no > offset. > Your concern would apply if we were dealing with more general polygons. Acknowledged. https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_constraints_util_sets.h (right): https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:187: double width_; On 2017/03/06 11:08:23, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > The point has no width and height, only coordinates, x and y are better names > > for that I think. > > I like height and width better to avoid confusion with aspect ratios and other > code where it's more convenient to think of the points as screen resolutions. > Although the Point class has generic 2D-vector operations, that's just a > byproduct of the approach chosen to solve the problem of distance to ideal. If > we were to change the algorithm, the Point class might not need to be so much > like a vector, but it would still have width, height and aspect ratio. > > Perhaps we should hide the vector-like operations in the .cc file, even at the > cost of direct testability. The way I see it is: Input to the algorithm is width and height but this class is used as a point, with distance calculations, line segments, polygons and vector math, it's fairly generic. It's even named Point. But this is nitty. If you want to keep it as-is could you add a TODO saying if a Vector2dD becomes available use that instead? https://codereview.chromium.org/2728633002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_constraints_util_sets.h:257: // the point closest to (|ideal_height|, |ideal_width|) is returned. On 2017/03/06 11:08:23, Guido Urdaneta wrote: > On 2017/03/03 16:01:42, hbos_chromium wrote: > > It doesn't matter but it surprised/confused me at first: Why heigh-width > > Cartesian space and not width-height? > > If point (width,height) is replaced by vector (x,y) then it should be > > width-height aka x-y space instead of y-x or else it would be confusing. > > Because it is easier to think of aspectRatio as a line of the form > width = aspectRatio * height, > and have the min aspectRatio be the line below and the max aspectRatio the line > above in the Cartesian plane. > > The difficulty of thinking of aspect ratio as the inverse of the slope of the > lines that make up the polygon is a lot worse than having height on the X axis. > Also, although having width on the X axis sounds natural because that's how it > is on a screen, such a Cartesian plane would not really mimic a screen, but a > set of screen sizes. Acknowledged. If this at any point is replaced by a vector class (x,y) then you could translate "width, height" to p = (height, width) and continue to use y = aspectRatio * x for lines. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets.cc:470: // Start with min_height, min_width and continue along min_width. nit: Parenthesis around points, "Start with (min_height, min_width) and continue along min_width."
lgtm https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:53: // polygon, using the first vertex as reference. Because this test uses the first vertex as the reference point it could fail for concave polygons that are in fact in counterclockwise order. We don't really care about that but we never actually test that the polygon is convex, which maybe we should? Either: 1. Update the test to check for both vertex order and convexivity. You can do this by looking at the vector of one edge and the next edge. Rotate the first edge's vector by 90 degrees and take the dot product with the second vector, the sign determines if that edge turns "inwards" or "outwards". If the dot products calculated as such for all edges all have the same sign the polygon is convex. If the order is clockwise or counterclockwise can be determined by the sign. I'm doing this from the top of my head but it's something like: e1 = (v[i + 1] - v[i]) e2 = (v[i + 2] - v[i + 1]) e1Perpendicular = (-e1.y, e1.x) s = e2 dot e1Perpendicular direction = sign(s) Precision warning: if s is approximately zero we should not use its sign to determine direction because the lines are approximately parallel. 2. Update the comment to say that we assume that the polygon is convex and it may yield false results otherwise. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:65: return false; I think this is prone to rounding errors. Parallel or nearly parallel lines could make this return false unless we have friendly numbers. Things like this could be a reason for needing double instead of float for tests to pass? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:396: EXPECT_TRUE(intersection.ContainsPoint(2, 1)); How about EXPECT-ing the ResolutionSet to be approximately (2, 2, 1, 1, 0.5, 0.5)? And similarly check the result of the intersection by looking at its values rather than checking if points are contained with it. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:398: // It should not contain any point in the vicinity of the included point. nit: It's good if the vicinity is super close and not a unit along an axis away from set1. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:502: P(100, 100), P(100 / 1.5, 100), P(10, 15)}; EXPECT_EQ(vertices, set.ComputeVertices()) https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:548: ResolutionSet set(100, 1000, 100, 1000, 0.5, 2.0); EXPECT_EQ the vertices returned by ComputeVertices? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:635: ResolutionSet set(100, 1000, 100, 1000, 0.0, 1.0); EXPECT_EQ the vertices returned by ComputeVertices? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:711: factory_.basic().aspectRatio.setIdeal(kIdealAspectRatio); Comment saying the ideal aspect ratio doesn't matter when w and h are supplied? Also this aspect ratio != w/h. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:722: ResolutionSet set(100, 1000, 100, 1000, 0.0, 1.0); EXPECT_EQ the vertices returned by ComputeVertices? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:766: MediaStreamVideoSource::kDefaultWidth), EXPECT_EQ that this point is inside the set. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:820: TEST_F(MediaStreamConstraintsUtilSetsTest, ResolutionVertices) { Should VerticesContains be replaced by vertices which are tested for both equality with ComputeVertices and that they are contained? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:1116: // Unconstrained set. Here and elsewhere: "unconstrained" -> "universal", "constrained" -> "explicit elements"?, etc
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
hbos: PTAL still lgtm? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets.cc:470: // Start with min_height, min_width and continue along min_width. On 2017/03/08 21:03:00, hbos_chromium wrote: > nit: Parenthesis around points, "Start with (min_height, min_width) and continue > along min_width." Done. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:45: // Single-segment, single point or single segment are trivial cases. On 2017/03/08 12:42:51, hta - Chromium wrote: > Nit: remove repeated "single-segment" Done. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:53: // polygon, using the first vertex as reference. On 2017/03/09 11:34:22, hbos_chromium wrote: > Because this test uses the first vertex as the reference point it could fail for > concave polygons that are in fact in counterclockwise order. We don't really > care about that but we never actually test that the polygon is convex, which > maybe we should? > > Either: > > 1. Update the test to check for both vertex order and convexivity. You can do > this by looking at the vector of one edge and the next edge. Rotate the first > edge's vector by 90 degrees and take the dot product with the second vector, the > sign determines if that edge turns "inwards" or "outwards". If the dot products > calculated as such for all edges all have the same sign the polygon is convex. > If the order is clockwise or counterclockwise can be determined by the sign. > > I'm doing this from the top of my head but it's something like: > > e1 = (v[i + 1] - v[i]) > e2 = (v[i + 2] - v[i + 1]) > e1Perpendicular = (-e1.y, e1.x) > s = e2 dot e1Perpendicular > direction = sign(s) > > Precision warning: if s is approximately zero we should not use its sign to > determine direction because the lines are approximately parallel. > > 2. Update the comment to say that we assume that the polygon is convex and it > may yield false results otherwise. Your solution to 1 is in principle correct (check that every interior angle is less than 180 degrees), but it assumes that vertices come in the right order (2 consecutive vertices are a segment). However, the check that they come in the right order assumes that the polygon is convex, so there is no value in checking for that after it was already an assumption. An alternative solution would be to check convexity by finding the convex hull of the set of vertices returned by ComputeVertices and verify that every vertex is in the convex hull, and then do the check for counterclockwise order. I think that's a bit overkill here, so I'm going for option 2 and updating the comment :) https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:65: return false; On 2017/03/09 11:34:22, hbos_chromium wrote: > I think this is prone to rounding errors. Parallel or nearly parallel lines > could make this return false unless we have friendly numbers. > > Things like this could be a reason for needing double instead of float for tests > to pass? Acknowledged. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:396: EXPECT_TRUE(intersection.ContainsPoint(2, 1)); On 2017/03/09 11:34:21, hbos_chromium wrote: > How about EXPECT-ing the ResolutionSet to be approximately (2, 2, 1, 1, 0.5, > 0.5)? > And similarly check the result of the intersection by looking at its values > rather than checking if points are contained with it. Checking set equality is tricky, because the correct test is that both sets are subsets of each other. Here, the result is going to be (1, 2, 1, 2, 0.5, 0.5), which contains exactly the same points as (2,2,1,1,0.5,0.5). Not sure if it's worth checking that the intersection has exactly one of the many possible configurations. This is the reason why the test was originally just checking element inclusion. WDYT? https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:398: // It should not contain any point in the vicinity of the included point. On 2017/03/09 11:34:22, hbos_chromium wrote: > nit: It's good if the vicinity is super close and not a unit along an axis away > from set1. Done. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:502: P(100, 100), P(100 / 1.5, 100), P(10, 15)}; On 2017/03/09 11:34:21, hbos_chromium wrote: > EXPECT_EQ(vertices, set.ComputeVertices()) I prefer to test that ComputeVertices works in ResolutionVertices. If you think that test doesn't have enough coverage, I can add additional cases there. This test originally defined the vertices manually to avoid calling ComputeVertices, but since this test is also friended because of ClosestPointTo, I updated the test to use it. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:548: ResolutionSet set(100, 1000, 100, 1000, 0.5, 2.0); On 2017/03/09 11:34:21, hbos_chromium wrote: > EXPECT_EQ the vertices returned by ComputeVertices? I prefer to test that in ResolutionVertices. If you think that test doesn't have enough coverage, I can add additional cases there. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:635: ResolutionSet set(100, 1000, 100, 1000, 0.0, 1.0); On 2017/03/09 11:34:21, hbos_chromium wrote: > EXPECT_EQ the vertices returned by ComputeVertices? Ditto. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:711: factory_.basic().aspectRatio.setIdeal(kIdealAspectRatio); On 2017/03/09 11:34:21, hbos_chromium wrote: > Comment saying the ideal aspect ratio doesn't matter when w and h are supplied? > Also this aspect ratio != w/h. Done. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:722: ResolutionSet set(100, 1000, 100, 1000, 0.0, 1.0); On 2017/03/09 11:34:21, hbos_chromium wrote: > EXPECT_EQ the vertices returned by ComputeVertices? Ditto. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:766: MediaStreamVideoSource::kDefaultWidth), On 2017/03/09 11:34:21, hbos_chromium wrote: > EXPECT_EQ that this point is inside the set. Done. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:820: TEST_F(MediaStreamConstraintsUtilSetsTest, ResolutionVertices) { On 2017/03/09 11:34:21, hbos_chromium wrote: > Should VerticesContains be replaced by vertices which are tested for both > equality with ComputeVertices and that they are contained? Changed AreValidVertices to check that the vertices are included in the set. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:1116: // Unconstrained set. On 2017/03/09 11:34:22, hbos_chromium wrote: > Here and elsewhere: "unconstrained" -> "universal", "constrained" -> "explicit > elements"?, etc Done.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Still lgtm! https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_sets_unittest.cc (right): https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:53: // polygon, using the first vertex as reference. On 2017/03/09 17:55:43, Guido Urdaneta wrote: > On 2017/03/09 11:34:22, hbos_chromium wrote: > > Because this test uses the first vertex as the reference point it could fail > for > > concave polygons that are in fact in counterclockwise order. We don't really > > care about that but we never actually test that the polygon is convex, which > > maybe we should? > > > > Either: > > > > 1. Update the test to check for both vertex order and convexivity. You can do > > this by looking at the vector of one edge and the next edge. Rotate the first > > edge's vector by 90 degrees and take the dot product with the second vector, > the > > sign determines if that edge turns "inwards" or "outwards". If the dot > products > > calculated as such for all edges all have the same sign the polygon is convex. > > If the order is clockwise or counterclockwise can be determined by the sign. > > > > I'm doing this from the top of my head but it's something like: > > > > e1 = (v[i + 1] - v[i]) > > e2 = (v[i + 2] - v[i + 1]) > > e1Perpendicular = (-e1.y, e1.x) > > s = e2 dot e1Perpendicular > > direction = sign(s) > > > > Precision warning: if s is approximately zero we should not use its sign to > > determine direction because the lines are approximately parallel. > > > > 2. Update the comment to say that we assume that the polygon is convex and it > > may yield false results otherwise. > > Your solution to 1 is in principle correct (check that every interior angle is > less than 180 degrees), but it assumes that vertices come in the right order (2 > consecutive vertices are a segment). However, the check that they come in the > right order assumes that the polygon is convex, so there is no value in > checking for that after it was already an assumption. > An alternative solution would be to check convexity by finding the convex hull > of the set of vertices returned by ComputeVertices and verify that every vertex > is in the convex hull, and then do the check for counterclockwise order. > I think that's a bit overkill here, so I'm going for option 2 and updating the > comment :) I think the only case 1) doesn't guard against is when the lines go around in a loop (and self-intersect), i.e. we "go around" in multiple revolutions when we follow the lines. As long as that is not the case this is a valid convex/concave check I believe. If you want it to be bullet-proof against that special case too you can also make sure that a point known to be on the interior (e.g. any one of the vertices) is always on the inside of each edge (simple, just another vector-dot test per edge). Or test every edge against intersection with other (non-neighbor) edges (slightly more complicated, requiring a line-line intersection test). But we can skip this. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:65: return false; On 2017/03/09 17:55:43, Guido Urdaneta wrote: > On 2017/03/09 11:34:22, hbos_chromium wrote: > > I think this is prone to rounding errors. Parallel or nearly parallel lines > > could make this return false unless we have friendly numbers. > > > > Things like this could be a reason for needing double instead of float for > tests > > to pass? > > Acknowledged. How about <= kTolerance? In case of future tests failing or regression when updating something. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:502: P(100, 100), P(100 / 1.5, 100), P(10, 15)}; On 2017/03/09 17:55:43, Guido Urdaneta wrote: > On 2017/03/09 11:34:21, hbos_chromium wrote: > > EXPECT_EQ(vertices, set.ComputeVertices()) > > I prefer to test that ComputeVertices works in ResolutionVertices. If you think > that test doesn't have enough coverage, I can add additional cases there. > This test originally defined the vertices manually to avoid calling > ComputeVertices, but since this test is also friended because of ClosestPointTo, > I updated the test to use it. Acknowledged. https://codereview.chromium.org/2728633002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_sets_unittest.cc:548: ResolutionSet set(100, 1000, 100, 1000, 0.5, 2.0); On 2017/03/09 17:55:43, Guido Urdaneta wrote: > On 2017/03/09 11:34:21, hbos_chromium wrote: > > EXPECT_EQ the vertices returned by ComputeVertices? > > I prefer to test that in ResolutionVertices. If you think that test doesn't have > enough coverage, I can add additional cases there. Acknowledged.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org, hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2728633002/#ps140001 (title: "fix Windows compile issues + comments/formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
guidou@chromium.org changed reviewers: + avi@chromium.org
avi@: Can you rs?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
avi@: can you rs this one as well for the BUILD.gn files?
lgtm stamp
The CQ bit was checked by guidou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489597566934310,
"parent_rev": "8be5c3fbeef96d6e423f4e468b6d3c49fe99490a", "commit_rev":
"ebf283a6e622fdd02d0ab9ee2386dfa50ebcc4e3"}
Message was sent while issue was closed.
Description was changed from ========== Add utility set classes to support getUserMedia constraint proccessing. See https://w3c.github.io/mediacapture-main/#constrainable-interface This CL will be followed by CLs implementing algorithms that use the classes provided by this CL. BUG=657733 ========== to ========== Add utility set classes to support getUserMedia constraint proccessing. See https://w3c.github.io/mediacapture-main/#constrainable-interface This CL will be followed by CLs implementing algorithms that use the classes provided by this CL. BUG=657733 Review-Url: https://codereview.chromium.org/2728633002 Cr-Commit-Position: refs/heads/master@{#457137} Committed: https://chromium.googlesource.com/chromium/src/+/ebf283a6e622fdd02d0ab9ee2386... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ebf283a6e622fdd02d0ab9ee2386... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
