Closed Bug 884879 Opened 6 years ago Closed 6 years ago

[webvtt] Cue times are being rounded

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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

The behaviour we want is to not round.
Assignee: nobody → rick.eyre
Blocks: webvtt
Status: NEW → ASSIGNED
Depends on: 833386
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.
Depends on: 884884
No longer depends on: 833386
Attachment #764904 - Flags: review?(giles)
... Uploaded the wrong patch. This should be correct.
Attachment #764908 - Flags: review?(giles)
Attachment #764904 - Attachment is obsolete: true
Attachment #764904 - Flags: review?(giles)
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?
Attachment #764908 - Flags: review?(giles) → review-
Please note tests for this will now be landing in bug 833386 part 2.
Attachment #764908 - Attachment is obsolete: true
Attachment #765376 - Flags: review?(giles)
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. :)
Attachment #765376 - Flags: review?(giles) → review+
(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 ;).
Carrying forward r=rillian
Attachment #765376 - Attachment is obsolete: true
Well that was the incorrect link. Here is the correct one: https://tbpl.mozilla.org/?tree=Try&rev=9e2bfc68fa1e
Looks green so I'm marking checkin-needed.
Keywords: checkin-needed
Off by one in the bug number in the commit message.

https://hg.mozilla.org/mozilla-central/rev/386301e2ee28
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
No longer depends on: 884884
You need to log in before you can comment on or make changes to this bug.