1286444 - VideoPlaybackQuality stores uint64_t's when webidl interface may truncate to 32 bits

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

In VideoPlaybackQuality.webidl, the three frame-related attributes are "unsigned long", which translates to uint32_t.
However the corresponding VideoPlaybackQuality class members are stored and exposed as uint64_t.
So in the generated bindings file, these values are silently truncated down to 32 bits before being set in a JS number.

It is very unlikely to ever happen (2^32 frames @ 60Hz would take 2+ years), and it's "only" stats, but for peace of mind we should use the correct binding value types: unsigned long long.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1286444 - Use 64-bit numbers for VideoPlaybackQuality bindings -

totalVideoFrames, droppedVideoFrames, and corruptedVideoFrames were 'unsigned
long' (equivalent to uint32_t), but they come from the VideoPlaybackQuality
class, which stores and exposes uint64_t numbers.
So to avoid silent truncation, these attributes should use unsigned long long
(equivalent to uint64_t) instead.

Review commit: https://reviewboard.mozilla.org/r/63868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63868/

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8770393 [details]
Bug 1286444 - Use 64-bit numbers for VideoPlaybackQuality bindings -

https://reviewboard.mozilla.org/r/63868/#review60914

This is an interface from a standard, right?

I don't see any issues raised on the standard, and the standard says to do what we're doing right now.  If you think the spec should change, please raise a spec issue at the very least.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8770393 [details]
Bug 1286444 - Use 64-bit numbers for VideoPlaybackQuality bindings -

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8770393 [details]
> Bug 1286444 - Use 64-bit numbers for VideoPlaybackQuality bindings -
> 
> https://reviewboard.mozilla.org/r/63868/#review60914
> 
> This is an interface from a standard, right?
> 
> I don't see any issues raised on the standard, and the standard says to do
> what we're doing right now.  If you think the spec should change, please
> raise a spec issue at the very least.

Oh, right you are. Sorry, I should have checked that first!

Matthew, you wrote the VideoPlaybackQuality class in Bug 855130 (FF 25, June 2013), with uint64_t's while the webidl only exposes uint32_t's:
https://www.w3.org/TR/media-source/#videoplaybackquality

Would you recommend changing the class storage to 32-bit (with overflow-checking code if necessary) to match the standard interface, or trying to change the spec to 64 bits to allow bigger numbers?

(From what I could see in the code, I believe decoders only provide 32-bit values anyway, and there is no apparent accumulation that could cause overflows.)

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gerald Squelart [:gerald] from comment #3)
> (From what I could see in the code, I believe decoders only provide 32-bit
> values anyway, and there is no apparent accumulation that could cause
> overflows.)

To clarify:
- FrameStatistics does accumulate 32-bit numbers (from decoders) over time, so there could theoretically be overflows.
- But VideoPlaybackQuality itself is given its values only once (from FrameStatistics) at creation so there's no overflow possible there.

In the end we'll want to prevent overflows somewhere! (Right?)
Possibilities I can think of:
A. Store 32-bit values everywhere, and cap them at UINT32_MAX -- hoping it's an unlikely-enough case, where values don't mean much anymore.
B. Store 64-bit values everywhere and sanitize them (reduce them all in proportion to each other, to fit in 32 bits) before exposing them through the webidl interface.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Gerald Squelart [:gerald] from comment #3)
> Matthew, you wrote the VideoPlaybackQuality class in Bug 855130 (FF 25, June
> 2013), with uint64_t's while the webidl only exposes uint32_t's:
> https://www.w3.org/TR/media-source/#videoplaybackquality
> 
> Would you recommend changing the class storage to 32-bit (with
> overflow-checking code if necessary) to match the standard interface, or
> trying to change the spec to 64 bits to allow bigger numbers?

The former.

FWIW, the VPQ spec seems to be moving to here: https://wicg.github.io/media-playback-quality/

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1286444 - Store uint32_t's in VideoPlaybackQuality -

Because VideoPlaybackQuality attributes are exposed as 'unsigned long' in the
webidl interface, it would be dangerous to accept and store them as uint64_t
in the class, as silent truncation could happen on some systems where longs are
only 32 bit-long.

Review commit: https://reviewboard.mozilla.org/r/64146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64146/

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1286444 - Use correct types to construct VideoPlaybackQuality -

VideoPlaybackQuality was fed uint64_t's, now it should be given unsigned ints.
Note that FrameStatistics currently provide uint32_t's, so we are fine now;
nevertheless I added a static_assert to verify that and ensure it stays true.

Review commit: https://reviewboard.mozilla.org/r/64148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64148/

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1286444 - Fix types when using VideoPlaybackQuality values -

These numbers are only used to calculate a percentage, so they don't really
need to be 64-bit long.

Review commit: https://reviewboard.mozilla.org/r/64150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64150/

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8770814 [details]
Bug 1286444 - Store uint32_t's in VideoPlaybackQuality -

https://reviewboard.mozilla.org/r/64146/#review61198

r+ assuming s/unsigned long/uint32_t/

::: dom/media/VideoPlaybackQuality.h:25
(Diff revision 1)
>  public:
>    NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(VideoPlaybackQuality)
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(VideoPlaybackQuality)
>  
>    VideoPlaybackQuality(HTMLMediaElement* aElement, DOMHighResTimeStamp aCreationTime,
> -                       uint64_t aTotalFrames, uint64_t aDroppedFrames,
> +                       unsigned long aTotalFrames,

Per https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Integer_types (and if you look at the generated code in $objdir/dom/bindings/VideoPlaybackQualityBinding.cpp, e.g. the type of result in get_totalVideoFrames et al.) these should be uint32_t rather than unsigned long.

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 8770815 [details]
Bug 1286444 - Use correct types to construct VideoPlaybackQuality -

https://reviewboard.mozilla.org/r/64148/#review61200

r+ with uint32_t fix per previous patch comment

::: dom/html/HTMLVideoElement.cpp:246
(Diff revision 1)
>        }
>      }
>  
>      if (mDecoder) {
>        FrameStatistics& stats = mDecoder->GetFrameStatistics();
> +      static_assert(sizeof (unsigned long) >= sizeof(stats.GetParsedFrames()),

No space between sizeof and (

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8770816 [details]
Bug 1286444 - Fix types when using VideoPlaybackQuality values -

https://reviewboard.mozilla.org/r/64150/#review61202

r+ with uint32_t fixed, and then you can drop the uint32_t case in the percentage calculation

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://reviewboard.mozilla.org/r/64146/#review61198

> Per https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Integer_types (and if you look at the generated code in $objdir/dom/bindings/VideoPlaybackQualityBinding.cpp, e.g. the type of result in get_totalVideoFrames et al.) these should be uint32_t rather than unsigned long.

The MDN page confused me, as I can't find anything saying that in C99 stdint "unsigned long" equates to "uint32_t".
However the actual specs makes it clear: "The unsigned long type is an unsigned integer type that has values in the range [0, 4294967295]"
https://www.w3.org/TR/WebIDL/#idl-unsigned-long
And the generated binding obviously shows uint32_t.
So I'll use that.

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8770814 [details]
Bug 1286444 - Store uint32_t's in VideoPlaybackQuality -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64146/diff/1-2/

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8770815 [details]
Bug 1286444 - Use correct types to construct VideoPlaybackQuality -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64148/diff/1-2/

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8770816 [details]
Bug 1286444 - Fix types when using VideoPlaybackQuality values -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64150/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63c2807a4439
Store uint32_t's in VideoPlaybackQuality - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/4667487b0da2
Use correct types to construct VideoPlaybackQuality - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/9f1fccf586fb
Fix types when using VideoPlaybackQuality values - r=kinetik

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63c2807a4439
https://hg.mozilla.org/mozilla-central/rev/4667487b0da2
https://hg.mozilla.org/mozilla-central/rev/9f1fccf586fb