The default bug view has changed. See this FAQ.

[webvtt] Cue times are being rounded

RESOLVED FIXED in mozilla24

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: reyre, Assigned: reyre)

Tracking

(Blocks: 1 bug)

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

The behaviour we want is to not round.
(Assignee)

Updated

4 years ago
Assignee: nobody → rick.eyre
Blocks: 629350
Status: NEW → ASSIGNED
(Assignee)

Updated

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

Updated

4 years ago
Depends on: 884884
(Assignee)

Updated

4 years ago
No longer depends on: 833386
(Assignee)

Comment 2

4 years ago
Created attachment 764904 [details] [diff] [review]
v1: Make sure cue times are not rounded.
Attachment #764904 - Flags: review?(giles)
(Assignee)

Comment 3

4 years ago
Created attachment 764908 [details] [diff] [review]
v1: Make sure cue times are not rounded.

... Uploaded the wrong patch. This should be correct.
Attachment #764908 - Flags: review?(giles)
(Assignee)

Updated

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

Comment 5

4 years ago
Created attachment 765376 [details] [diff] [review]
v2: Make sure cue times aren't rounded

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+
(Assignee)

Comment 7

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

Comment 8

4 years ago
Created attachment 765463 [details] [diff] [review]
v3: Make sure cue times aren't rounded r=rillian

Carrying forward r=rillian
Attachment #765463 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #765376 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/try/rev/2f5ef80b8911
(Assignee)

Comment 10

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

Comment 11

4 years ago
Looks green so I'm marking checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/386301e2ee28
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Updated

4 years ago
No longer depends on: 884884
You need to log in before you can comment on or make changes to this bug.