Motown: Add new Ratio and LinearFunction classes to media/common/cpp.

Motown: Add new Ratio and LinearFunction classes to media/common/cpp.
Ratio and LinearFunction duplicate LinearTransform::Ratio and
LinearTransform with some improvements in terminology and notational
convenience. Unit tests are included. These classes have been added to
mojo/services/media/common/cpp so that clients can use them. The new
timing code will use these classes.

R=kulakowski@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/51a3952f63000f51dd6246a480b148efcd90174f

[dalesat, 2016-05-03 19:33:40]

Description was changed from

==========
Motown: Add new Ratio and LinearFunction classes to media/common/cpp.
Ratio and LinearFunction duplicate LinearTransform::Ratio and
LinearTransform with some improvements in terminology and notational
convenience. Unit tests are included. These classes have been added to
mojo/services/media/common/cpp so that clients can use them. The new
timing code will use these classes.
==========

to

==========
Motown: Add new Ratio and LinearFunction classes to media/common/cpp.
Ratio and LinearFunction duplicate LinearTransform::Ratio and
LinearTransform with some improvements in terminology and notational
convenience. Unit tests are included. These classes have been added to
mojo/services/media/common/cpp so that clients can use them. The new
timing code will use these classes.
==========

[dalesat, 2016-05-03 19:33:41]

dalesat@chromium.org changed reviewers:
+ kulakowski@chromium.org

[dalesat, 2016-05-03 19:33:54]

Please take a look. Thanks!

[kulakowski, 2016-05-03 20:16:17]

On 2016/05/03 19:33:54, dalesat wrote:
> Please take a look. Thanks!

I have one round of comments, mostly regarding the API.

[kulakowski, 2016-05-03 20:16:27]

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/linear_function.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:23: static int64_t
Apply(int64_t domain_basis,
In both LinearFunction and Ratio, I don't understand why you have a set of
public static members that take decomposed transforms / ratios, and then
nonstatic members as well that call into the former. Who else is supposed to use
the static ones?

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:89: int64_t domain_basis;
Here, and elsewhere, you should name member variables like_this_.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:18: T BinaryGcd(T a, T b) {
What's the motivation for doing it this way, vs the method with division?

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:26: static void Reduce(uint32_t*
numerator, uint32_t* denominator);
It doesn't make sense for this to be public. The invariant of this class, per
your comment above, is that a Ratio is reduced. If that's true, then any
operations modify or producing Ratios should already be reduced, and there
should be no need to ever call this as a client.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:40: static int64_t Scale(int64_t value,
uint32_t numerator, uint32_t denominator);
I don't understand why the value to scale by is signed if ratios are unsigned.

[dalesat, 2016-05-03 22:36:06]

PTAL

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/linear_function.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:23: static int64_t
Apply(int64_t domain_basis,
On 2016/05/03 20:16:26, kulakowski wrote:
> In both LinearFunction and Ratio, I don't understand why you have a set of
> public static members that take decomposed transforms / ratios, and then
> nonstatic members as well that call into the former. Who else is supposed to
use
> the static ones?

These classes are 'helpers', which to me means accommodating the use cases that
are likely to arise rather than enforcing a particular world view. Instances of
the classes are useful when the use case calls for variables, parameters, etc
that are actually of the provided types. The caller can build instances and play
with them as needed using instance methods, operators, etc. In other cases, the
caller just needs to leverage the core value of the classes, and actually having
to construct instances is a nuisance...hence the static methods.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:89: int64_t domain_basis;
On 2016/05/03 20:16:26, kulakowski wrote:
> Here, and elsewhere, you should name member variables like_this_.

OK. I've seen this done both ways for public member variables. Would it be
better to have accessor methods for all of them? At a minimum, these should be
const.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:18: T BinaryGcd(T a, T b) {
On 2016/05/03 20:16:26, kulakowski wrote:
> What's the motivation for doing it this way, vs the method with division?

This is purported to be 20% faster, according to my reading on the subject. YMMV
depending on processor math performance, etc.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:26: static void Reduce(uint32_t*
numerator, uint32_t* denominator);
On 2016/05/03 20:16:26, kulakowski wrote:
> It doesn't make sense for this to be public. The invariant of this class, per
> your comment above, is that a Ratio is reduced. If that's true, then any
> operations modify or producing Ratios should already be reduced, and there
> should be no need to ever call this as a client.

That's arguable. As discussed elsewhere, these static functions are for cases in
which the caller isn't otherwise motivated to create an instance of Ratio. If
you feel strongly about this, I can hide it.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:40: static int64_t Scale(int64_t value,
uint32_t numerator, uint32_t denominator);
On 2016/05/03 20:16:26, kulakowski wrote:
> I don't understand why the value to scale by is signed if ratios are unsigned.

While this class has a generic name (Ratio), it is designed with Motown in mind
(which is part of why it's in mojo::media and not somewhere more common). The
intended application is the manipulation of time values. Ratios themselves
represent rates, which are are always non-negative. Time values, on the other
hand, can be negative. This sometimes happens at the start of a media asset
when, due to alignment details, some packets are marked with negative
timestamps. There are also situations in which we want to reference points in
time before the beginning of content (e.g. preroll scenarios) and other
situations in which an origin for a particular timebase happens to be in the
future for some weird reason.

Also, uint32_t was chosen for ratios, because this is typically sufficient
resolution for rates. Timestamps, on the other hand, need to have e.g.
nanosecond resolution over timespans that exceed any practical need (in media).

[kulakowski, 2016-05-04 06:35:39]

Thanks for the explanation. Still a few small things.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/linear_function.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:23: static int64_t
Apply(int64_t domain_basis,
On 2016/05/03 22:36:06, dalesat wrote:
> On 2016/05/03 20:16:26, kulakowski wrote:
> > In both LinearFunction and Ratio, I don't understand why you have a set of
> > public static members that take decomposed transforms / ratios, and then
> > nonstatic members as well that call into the former. Who else is supposed to
> use
> > the static ones?
> 
> These classes are 'helpers', which to me means accommodating the use cases
that
> are likely to arise rather than enforcing a particular world view. Instances
of
> the classes are useful when the use case calls for variables, parameters, etc
> that are actually of the provided types. The caller can build instances and
play
> with them as needed using instance methods, operators, etc. In other cases,
the
> caller just needs to leverage the core value of the classes, and actually
having
> to construct instances is a nuisance...hence the static methods.

sgtm, this explanation helped motivate the patch and changes for me.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:26: static void Reduce(uint32_t*
numerator, uint32_t* denominator);
On 2016/05/03 22:36:06, dalesat wrote:
> On 2016/05/03 20:16:26, kulakowski wrote:
> > It doesn't make sense for this to be public. The invariant of this class,
per
> > your comment above, is that a Ratio is reduced. If that's true, then any
> > operations modify or producing Ratios should already be reduced, and there
> > should be no need to ever call this as a client.
> 
> That's arguable. As discussed elsewhere, these static functions are for cases
in
> which the caller isn't otherwise motivated to create an instance of Ratio. If
> you feel strongly about this, I can hide it.

Per my comment in the other place, I can believe that this is true. But then
please do not also document that Ratio "Expresses a non-negative rational number
as the *reduced* ratio".

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:40: static int64_t Scale(int64_t value,
uint32_t numerator, uint32_t denominator);
On 2016/05/03 22:36:06, dalesat wrote:
> On 2016/05/03 20:16:26, kulakowski wrote:
> > I don't understand why the value to scale by is signed if ratios are
unsigned.
> 
> While this class has a generic name (Ratio), it is designed with Motown in
mind
> (which is part of why it's in mojo::media and not somewhere more common). The
> intended application is the manipulation of time values. Ratios themselves
> represent rates, which are are always non-negative. Time values, on the other
> hand, can be negative. This sometimes happens at the start of a media asset
> when, due to alignment details, some packets are marked with negative
> timestamps. There are also situations in which we want to reference points in
> time before the beginning of content (e.g. preroll scenarios) and other
> situations in which an origin for a particular timebase happens to be in the
> future for some weird reason.
> 
> Also, uint32_t was chosen for ratios, because this is typically sufficient
> resolution for rates. Timestamps, on the other hand, need to have e.g.
> nanosecond resolution over timespans that exceed any practical need (in
media).

Ah, I was confused. I misread. As you can see, I wrote "... why the value to
scale *by*", which isn't right. It's "the value to scale". Thanks for the
explanation.

https://codereview.chromium.org/1950603002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/40001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:208: ?
-AsInt64(ScaleUInt64(static_cast<uint64_t>(-value), numerator,
Part of the reason I asked about the type of value is the undefined behavior
here.

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
File services/media/common/test/ratio_test.cc (right):

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
services/media/common/test/ratio_test.cc:23: 
Maybe EXPECT_whatever that gcd(numerator, denominator) == 1.

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
services/media/common/test/ratio_test.cc:126: VerifyScale(-1001, 2, 1, -2002);
And maybe some tests around INT64_MIN since that's always a tricky spot, as
mentioned elsewhere.

[dalesat, 2016-05-04 17:25:53]

PTAL

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/linear_function.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/linear_function.h:23: static int64_t
Apply(int64_t domain_basis,
On 2016/05/04 06:35:39, kulakowski wrote:
> On 2016/05/03 22:36:06, dalesat wrote:
> > On 2016/05/03 20:16:26, kulakowski wrote:
> > > In both LinearFunction and Ratio, I don't understand why you have a set of
> > > public static members that take decomposed transforms / ratios, and then
> > > nonstatic members as well that call into the former. Who else is supposed
to
> > use
> > > the static ones?
> > 
> > These classes are 'helpers', which to me means accommodating the use cases
> that
> > are likely to arise rather than enforcing a particular world view. Instances
> of
> > the classes are useful when the use case calls for variables, parameters,
etc
> > that are actually of the provided types. The caller can build instances and
> play
> > with them as needed using instance methods, operators, etc. In other cases,
> the
> > caller just needs to leverage the core value of the classes, and actually
> having
> > to construct instances is a nuisance...hence the static methods.
> 
> sgtm, this explanation helped motivate the patch and changes for me.

Acknowledged.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.h (right):

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:26: static void Reduce(uint32_t*
numerator, uint32_t* denominator);
On 2016/05/04 06:35:39, kulakowski wrote:
> On 2016/05/03 22:36:06, dalesat wrote:
> > On 2016/05/03 20:16:26, kulakowski wrote:
> > > It doesn't make sense for this to be public. The invariant of this class,
> per
> > > your comment above, is that a Ratio is reduced. If that's true, then any
> > > operations modify or producing Ratios should already be reduced, and there
> > > should be no need to ever call this as a client.
> > 
> > That's arguable. As discussed elsewhere, these static functions are for
cases
> in
> > which the caller isn't otherwise motivated to create an instance of Ratio.
If
> > you feel strongly about this, I can hide it.
> 
> Per my comment in the other place, I can believe that this is true. But then
> please do not also document that Ratio "Expresses a non-negative rational
number
> as the *reduced* ratio".

Done.

https://codereview.chromium.org/1950603002/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.h:40: static int64_t Scale(int64_t value,
uint32_t numerator, uint32_t denominator);
On 2016/05/04 06:35:39, kulakowski wrote:
> On 2016/05/03 22:36:06, dalesat wrote:
> > On 2016/05/03 20:16:26, kulakowski wrote:
> > > I don't understand why the value to scale by is signed if ratios are
> unsigned.
> > 
> > While this class has a generic name (Ratio), it is designed with Motown in
> mind
> > (which is part of why it's in mojo::media and not somewhere more common).
The
> > intended application is the manipulation of time values. Ratios themselves
> > represent rates, which are are always non-negative. Time values, on the
other
> > hand, can be negative. This sometimes happens at the start of a media asset
> > when, due to alignment details, some packets are marked with negative
> > timestamps. There are also situations in which we want to reference points
in
> > time before the beginning of content (e.g. preroll scenarios) and other
> > situations in which an origin for a particular timebase happens to be in the
> > future for some weird reason.
> > 
> > Also, uint32_t was chosen for ratios, because this is typically sufficient
> > resolution for rates. Timestamps, on the other hand, need to have e.g.
> > nanosecond resolution over timespans that exceed any practical need (in
> media).
> 
> Ah, I was confused. I misread. As you can see, I wrote "... why the value to
> scale *by*", which isn't right. It's "the value to scale". Thanks for the
> explanation.

Acknowledged.

https://codereview.chromium.org/1950603002/diff/40001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/40001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:208: ?
-AsInt64(ScaleUInt64(static_cast<uint64_t>(-value), numerator,
On 2016/05/04 06:35:39, kulakowski wrote:
> Part of the reason I asked about the type of value is the undefined behavior
> here.

Done.

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
File services/media/common/test/ratio_test.cc (right):

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
services/media/common/test/ratio_test.cc:23: 
On 2016/05/04 06:35:39, kulakowski wrote:
> Maybe EXPECT_whatever that gcd(numerator, denominator) == 1.

Done.

https://codereview.chromium.org/1950603002/diff/40001/services/media/common/t...
services/media/common/test/ratio_test.cc:126: VerifyScale(-1001, 2, 1, -2002);
On 2016/05/04 06:35:39, kulakowski wrote:
> And maybe some tests around INT64_MIN since that's always a tricky spot, as
> mentioned elsewhere.

Done.

[kulakowski, 2016-05-04 17:41:15]

Otherwise lg2m

https://codereview.chromium.org/1950603002/diff/60001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/60001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:196:
ScaleUInt64(static_cast<uint64_t>(std::abs(value)), numerator,
Sadly, this just pushed the UD into std::abs(value).

[dalesat, 2016-05-04 18:05:04]

PTAL

https://codereview.chromium.org/1950603002/diff/60001/mojo/services/media/com...
File mojo/services/media/common/cpp/ratio.cc (right):

https://codereview.chromium.org/1950603002/diff/60001/mojo/services/media/com...
mojo/services/media/common/cpp/ratio.cc:196:
ScaleUInt64(static_cast<uint64_t>(std::abs(value)), numerator,
On 2016/05/04 17:41:15, kulakowski wrote:
> Sadly, this just pushed the UD into std::abs(value).

Done.

[kulakowski, 2016-05-04 18:27:42]

lgtm

[dalesat, 2016-05-04 18:31:30]

Description was changed from

==========
Motown: Add new Ratio and LinearFunction classes to media/common/cpp.
Ratio and LinearFunction duplicate LinearTransform::Ratio and
LinearTransform with some improvements in terminology and notational
convenience. Unit tests are included. These classes have been added to
mojo/services/media/common/cpp so that clients can use them. The new
timing code will use these classes.
==========

to

==========
Motown: Add new Ratio and LinearFunction classes to media/common/cpp.
Ratio and LinearFunction duplicate LinearTransform::Ratio and
LinearTransform with some improvements in terminology and notational
convenience. Unit tests are included. These classes have been added to
mojo/services/media/common/cpp so that clients can use them. The new
timing code will use these classes.

R=kulakowski@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/51a3952f63000f51dd6246a480b...
==========

[dalesat, 2016-05-04 18:31:31]

Committed patchset #6 (id:100001) manually as
51a3952f63000f51dd6246a480b148efcd90174f (presubmit successful).