884879 - [webvtt] Cue times are being rounded

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

Cue times with decimals, i.e. 00:03.500, is rounded to 3.

The behaviour we want is to not round.

Comment 1

[Caitlin Potter (:caitp)]

not like there's going to be any shocking feedback or anything -- just make sure the milliseconds are converted to a real number before dividing by 1000.

Comment 2

[Rick Eyre (:reyre)]

Attachment: v1: Make sure cue times are not rounded.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v1: Make sure cue times are not rounded.

... Uploaded the wrong patch. This should be correct.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 764908 [details] [diff] [review]
v1: Make sure cue times are not rounded.

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

Thanks for writing tests. However this seems like something which could be combined with existing tests in 833386.

::: content/media/WebVTTLoadListener.cpp
@@ +160,4 @@
>  
>    nsRefPtr<TextTrackCue> textTrackCue =
>      new TextTrackCue(mElement->OwnerDoc()->GetParentObject(),
> +                     SECONDS_TO_MS((double)aCue->from),

Safer to make SECONDS_TO_MS do the cast?

Comment 5

[Rick Eyre (:reyre)]

Attachment: v2: Make sure cue times aren't rounded

Please note tests for this will now be landing in bug 833386 part 2.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 765376 [details] [diff] [review]
v2: Make sure cue times aren't rounded

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

Thanks for moving the cast to the macro. r=me with the precedence question addressed.

::: content/media/VideoUtils.h
@@ +164,4 @@
>  static const int64_t USECS_PER_MS = 1000;
>  
>  // Converts seconds to milliseconds.
> +#define MS_TO_SECONDS(s) (((double)s) / PR_MSEC_PER_SEC)

Is this safe against precedence confusion? I would have written it ((double)(s) / PR_MSEC_PER_SEC).

or ((double)(s) / (PR_MSEC_PER_SEC)) if you want to be paranoid. :)

Comment 7

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #6)
> Is this safe against precedence confusion? I would have written it
> ((double)(s) / PR_MSEC_PER_SEC).
> 
> or ((double)(s) / (PR_MSEC_PER_SEC)) if you want to be paranoid. :)

I was thinking this would ensure that we cast before division... but I guess that's not true because casting has higher precedence than division. I'll change it to the paranoid way ;).

Comment 8

[Rick Eyre (:reyre)]

Attachment: v3: Make sure cue times aren't rounded r=rillian

Carrying forward r=rillian

Comment 9

[Rick Eyre (:reyre)]

https://hg.mozilla.org/try/rev/2f5ef80b8911

Comment 10

[Rick Eyre (:reyre)]

Well that was the incorrect link. Here is the correct one: https://tbpl.mozilla.org/?tree=Try&rev=9e2bfc68fa1e

Comment 11

[Rick Eyre (:reyre)]

Looks green so I'm marking checkin-needed.

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/386301e2ee28

Comment 13

[Phil Ringnalda (:philor)]

Off by one in the bug number in the commit message.

https://hg.mozilla.org/mozilla-central/rev/386301e2ee28