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

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

50 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8770393 [details]
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/
Attachment #8770393 - Flags: review?(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.
Attachment #8770393 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 3

2 years ago
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.)
Attachment #8770393 - Attachment is obsolete: true
Flags: needinfo?(kinetik)
(Assignee)

Comment 4

2 years ago
(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.
(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/
Flags: needinfo?(kinetik)
(Assignee)

Comment 6

2 years ago
Created attachment 8770814 [details]
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/
Attachment #8770814 - Flags: review?(kinetik)
Attachment #8770815 - Flags: review?(kinetik)
Attachment #8770816 - Flags: review?(kinetik)
(Assignee)

Comment 7

2 years ago
Created attachment 8770815 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8770816 [details]
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/
(Assignee)

Updated

2 years ago
Summary: VideoPlaybackQuality webidl is truncating values down to 32 bits → VideoPlaybackQuality stores uint64_t's when webidl interface may truncate to 32 bits
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.
Attachment #8770814 - Flags: review?(kinetik) → review+
Attachment #8770815 - Flags: review?(kinetik) → review+
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 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
Attachment #8770816 - Flags: review?(kinetik) → review+
(Assignee)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
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/
Attachment #8770814 - Attachment description: Bug 1286444 - Store unsigned ints in VideoPlaybackQuality - → Bug 1286444 - Store uint32_t's in VideoPlaybackQuality -
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Comment 15

2 years ago
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

2 years ago
Pushed by gsquelart@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63c2807a4439
https://hg.mozilla.org/mozilla-central/rev/4667487b0da2
https://hg.mozilla.org/mozilla-central/rev/9f1fccf586fb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.