646323 - Add floating point support to mfbt/Casting.h use it for numeric casts in content/media

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Matthew Gregan [:kinetik]]

There are a bunch of places in the media code where we cast (either old-style or static_cast) numeric types to smaller types, or from signed to unsigned types.  To ensure these are safe, it's necessary to check that the value being cast is in the range of the target type.

Boost's numeric_cast does roughly what we want, so let's implement a simplified version that handles the conversions we use and included it in VideoUtils.h.  If it turns out to be more generally useful we can move it into xpcom/base sometime later.

xpcom/ds/CheckedInt.h already provides most of the framework for this (including a replacement for std::numeric_limits, which apparently can't be used with PR types).

Comment 1

[peyrard.max43]

Attachment: Extension of mfbt/Casting.h to Floating type for overflow detection. Application and test in content/media

The patch is long because of the use of SafeCast in content/media the tests begin at line 765  and the code for Casting.h at 1530

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 759788 [details] [diff] [review]
Extension of mfbt/Casting.h to Floating type for overflow detection. Application and test in content/media

Review of attachment 759788 [details] [diff] [review]:
-----------------------------------------------------------------

Before we get to adding in floating-point behavior at all, you're misunderstanding how SafeCast works.  It takes a value of From type, asserts that that value is in To type's range, then casts to To.  (The current description is a little misleading -- it probably shouldn't talk about "safe cast" at all, only about whether the input value exists in the output type.)  But I see several casts here along the lines of SafeCast<uint16_t>(-1) -- and -1 is clearly not in uint16_t range.  So at least some of the casts here -- I saw some, probably missed others skimming -- are definitely wrong, and should remain as static_cast<>.

Past that, I have concerns about your overall approach.  The existing SafeCast allows the cast *only* if the value will remain unchanged.  This is *not* the same as whether the cast only invokes defined behavior.  For example, static_cast<unsigned>(-1) is perfectly well-defined, but SafeCast<unsigned>(-1) is verboten.  Your patch checks whether the cast will only invoke defined behavior.  (I think; I haven't puzzled through all its intricacies, because of the other issues I see here.)  I'm pretty sure you accept SafeCast<int>(5.5), for example.

I think we probably don't want SafeCast to truncate.  That's an actual numeric change; a method that may change the input value should be clear that it might do so.  Adding

template<typename To, typename From>
inline To
Truncate(const From from);

to FloatingPoint.h, that returns a truncated value (and asserts if given Nan or an infinity or a number that, after truncation, is out of range)  seems preferable to me.  If SafeCast should accept floating point types as well as integral, I think it should only do so if the input value is exactly the output value.

Assuming you want to push forward with making SafeCast accept floating-point types so long as the conversion would be exact, and with some sort of separate Truncate method might change the input value, in the process of converting it, I'd appreciate if you kept the Casting.h/TestCasting.cpp changes in a separate patch from all the uses.  I'd like to look at both, but it'd be nice to sort through the implementation bits without being distracted by the uses at the same time.

::: content/media/gstreamer/GStreamerFormatHelper.cpp
@@ +53,4 @@
>  
>  GStreamerFormatHelper::GStreamerFormatHelper()
>    : mFactories(nullptr),
> +    mCookie(SafeCast<uint32_t>(-1))

Not actually okay.  :-)

::: content/media/ogg/OggCodecState.cpp
@@ +41,4 @@
>  {
>    uint32_t lo = LEUint32(p);
>    uint32_t hi = LEUint32(p + 4);
> +  return SafeCast<int64_t>(lo) | (SafeCast<int64_t>(hi) << 32);

Pre-existing, but if |hi| has its highest bit set, this invokes undefined behavior.  Shifting a 1 into (or past) the sign bit in a signed type is a no-no.

More fundamentally, this method shouldn't exist.  This code wants mozilla::LittleEndian::readInt64 instead from mozilla/Endian.h.  All the callers should just be replaced with calls to that.  Same for LEUint32, LEUint16, and LEInt16.

@@ +53,4 @@
>  // Reads a little-endian encoded signed 16bit integer at p.
>  static int16_t LEInt16(const unsigned char* p)
>  {
> +  return SafeCast<int16_t>(LEUint16(p));

Another misapprehension of what SafeCast does, which will assert if the high bit of the uint16_t that's read is set.

::: content/media/omx/OmxDecoder.cpp
@@ +120,4 @@
>  status_t MediaStreamSource::getSize(off64_t *size)
>  {
>    uint64_t length = mResource->GetLength();
> +  if (length == SafeCast<uint64_t>(-1))

Won't work.

::: content/media/test/TestSafeCast.cpp
@@ +1,1 @@
> +#include "../../../xpcom/tests/TestHarness.h"

This should be in mfbt/tests/, not here, following the pattern of those files.

::: content/media/wave/WaveReader.cpp
@@ +103,4 @@
>    int16_t
>    ReadInt16LE(const char** aBuffer)
>    {
> +    return SafeCast<int16_t>(ReadUint16LE(aBuffer));

All the methods here should be implemented in terms of mozilla::LittleEndian::readSomething.

::: mfbt/Casting.h
@@ +159,5 @@
>  };
>  
> +
> +// Conversion from floating point type to integral type
> +enum FIToSigned { FIToIsSigned, FIToIsNotSigned };

enum FToIntegral { FToSignedI, FToUnsignedI } would be better.

@@ +162,5 @@
> +// Conversion from floating point type to integral type
> +enum FIToSigned { FIToIsSigned, FIToIsNotSigned };
> +
> +template<typename From, typename To,
> +         bool FIToSigned = IsSigned<To>::value ? FIToIsSigned : FIToIsNotSigned>

Use FToIntegral as the type of the param, not bool.

Comment 3

[Jeff Walden [:Waldo]]

Nathan, do my thoughts on extending SafeCast to take To/From as floating point types, and adding a separate Truncate method (or some other name -- that's just what came to mind first) that might also change the input value in the conversion process, seem reasonable to you?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Nathan, do my thoughts on extending SafeCast to take To/From as floating
> point types, and adding a separate Truncate method (or some other name --
> that's just what came to mind first) that might also change the input value
> in the conversion process, seem reasonable to you?

I think you only want to enable cases in SafeCast for integer to floating point and floating point to floating point.  Floating point to integer conversions should be forced to use Truncate so it's a little more obvious what's going on.  No situations where SafeCast would be preferred over Truncate in floating point to integer conversions come to mind, though I'd be happy to be shown otherwise.

I suppose Truncate should also support converting from integer/floating point values to floating point too.  Although, hm, I see from reading my C++ draft that it's implementation-defined whether you chose the lower or higher bound if your source value falls between two representable floating point numbers (4.8p1, 4.9p2).  Ugh.  Can we get by without those cases for the time being?

My bikeshed has Truncate in Casting.h instead of FloatingPoint.h.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> I think you only want to enable cases in SafeCast for integer to floating
> point and floating point to floating point.  Floating point to integer
> conversions should be forced to use Truncate so it's a little more obvious
> what's going on.

Yeah, that makes sense.

> No situations where SafeCast would be preferred over
> Truncate in floating point to integer conversions come to mind, though I'd
> be happy to be shown otherwise.

And that.

> I suppose Truncate should also support converting from integer/floating
> point values to floating point too.  Although, hm, I see from reading my C++
> draft that it's implementation-defined whether you chose the lower or higher
> bound if your source value falls between two representable floating point
> numbers (4.8p1, 4.9p2).  Ugh.  Can we get by without those cases for the
> time being?

Sounds good to me.  (I'm guessing the implementation-defined bit is a nod to configurable FPU rounding modes, so it's probably not as bad as it sounds in practice.  Still, best avoided if we can.)

> My bikeshed has Truncate in Casting.h instead of FloatingPoint.h.

It's a little nitpicky.  Putting floating point methods in Casting.h probably means the format-constants like DoubleExponentBias and such would have to move to Casting.h, which feels really weird.  I dunno.  Maybe they have to move to implement even the restrictive bits of SafeCast you/we suggest.  Blargh.

Comment 6

[Jeff Walden [:Waldo]]

FWIW as to double-to-float casts, mozilla::IsFloatRepresentable now exists as the necessary primitive to use to test that double-to-float casting will not change value, and it could be integrated into Casting.h support for AssertedCast of floating-to-floating.

And as to float-to-integer casts, mozilla::Number{Is,Equals}Int32 exist to support verifying that float-to-int32_t casts won't change value.  But the underlying algorithm is template-defined in integer-type-agnostic fashion, so it should be easy to add different-width variations as needed.  And the template algorithm itself could be exposed publicly (currently it's in mozilla::detail) if a templated version were helpful.

But there remains no well-defined truncating operation, if such is actually desired in any of this.  (Memory hazy, I'm just skimming comments here.)

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jimm, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 646323 - Rewrite mfbt/Casting.h assertion in modern style, and teach it to deal with floating point values. r?kinetik

This now uses if constexpr (...) which is a lot more readable, and still
compiles to almost no assembly instructions, as expected.

Floating point casting assert when casting an integer that's too large to be
represented exactly as a floating point (e.g. UINT64_MAX to double, since double
have less than 64 bytes of mantissa), or when casting a double that's too large
to be represented in a float.

Depends on D167914

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39cfeb7cffda
Rewrite mfbt/Casting.h assertion in modern style, and teach it to deal with floating point values. r=kinetik

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 646323 - Add missing GCC diagnostic push.

Comment 11

[Cristina Horotan [:chorotan]]

Backed out changeset 39cfeb7cffda (bug 646323) for causing bustages

Backout: https://hg.mozilla.org/integration/autoland/rev/7075e948c78f7be501e9d2e277207df36628f59e

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=IzK74X8rSjiJACSFuN7CHg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=39cfeb7cffda4d1ae0338fbdd4771232afa880cf

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=404223455&repo=autoland&lineNumber=830
https://treeherder.mozilla.org/logviewer?job_id=404223447&repo=autoland&lineNumber=4806

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bf5abdd5b4f
Rewrite mfbt/Casting.h assertion in modern style, and teach it to deal with floating point values. r=kinetik

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8bf5abdd5b4f