Last Comment Bug 884879 - [webvtt] Cue times are being rounded
: [webvtt] Cue times are being rounded
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Rick Eyre (:reyre)
:
Mentors:
Depends on:
Blocks: webvtt
  Show dependency treegraph
 
Reported: 2013-06-19 09:18 PDT by Rick Eyre (:reyre)
Modified: 2013-06-24 19:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: Make sure cue times are not rounded. (1.31 KB, patch)
2013-06-19 11:48 PDT, Rick Eyre (:reyre)
no flags Details | Diff | Review
v1: Make sure cue times are not rounded. (4.24 KB, patch)
2013-06-19 11:51 PDT, Rick Eyre (:reyre)
giles: review-
Details | Diff | Review
v2: Make sure cue times aren't rounded (739 bytes, patch)
2013-06-20 07:34 PDT, Rick Eyre (:reyre)
giles: review+
Details | Diff | Review
v3: Make sure cue times aren't rounded r=rillian (750 bytes, patch)
2013-06-20 11:12 PDT, Rick Eyre (:reyre)
giles: review+
Details | Diff | Review

Description Rick Eyre (:reyre) 2013-06-19 09:18:37 PDT
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) 2013-06-19 09:24:09 PDT
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) 2013-06-19 11:48:33 PDT
Created attachment 764904 [details] [diff] [review]
v1: Make sure cue times are not rounded.
Comment 3 Rick Eyre (:reyre) 2013-06-19 11:51:28 PDT
Created attachment 764908 [details] [diff] [review]
v1: Make sure cue times are not rounded.

... Uploaded the wrong patch. This should be correct.
Comment 4 Ralph Giles (:rillian) needinfo me 2013-06-19 12:57:20 PDT
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) 2013-06-20 07:34:43 PDT
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.
Comment 6 Ralph Giles (:rillian) needinfo me 2013-06-20 09:12:46 PDT
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) 2013-06-20 09:23:07 PDT
(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) 2013-06-20 11:12:38 PDT
Created attachment 765463 [details] [diff] [review]
v3: Make sure cue times aren't rounded r=rillian

Carrying forward r=rillian
Comment 9 Rick Eyre (:reyre) 2013-06-20 11:44:29 PDT
https://hg.mozilla.org/try/rev/2f5ef80b8911
Comment 10 Rick Eyre (:reyre) 2013-06-20 17:57:02 PDT
Well that was the incorrect link. Here is the correct one: https://tbpl.mozilla.org/?tree=Try&rev=9e2bfc68fa1e
Comment 11 Rick Eyre (:reyre) 2013-06-21 07:02:20 PDT
Looks green so I'm marking checkin-needed.
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2013-06-22 05:49:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/386301e2ee28
Comment 13 Phil Ringnalda (:philor) 2013-06-22 16:04:55 PDT
Off by one in the bug number in the commit message.

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

Note You need to log in before you can comment on or make changes to this bug.