Closed Bug 1193603 Opened 4 years ago Closed 4 years ago

Miscalculation in MediaMetadataManager::DispatchMetadataIfNeeded

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(2 files)

https://hg.mozilla.org/mozilla-central/file/38c1ea9ccae3/dom/media/MediaMetadataManager.h#l48

aCurrentTime is in micro seconds. metadata->mPublishTime shouldn't be converted to seconds when comparing to aCurrentTime.
Bug 1193603. Part 1 - Fix indentation and include order.
Attachment #8646731 - Flags: review?(jyavenard)
Bug 1193603. Part 2 - Fix miscalculation in converting micro seconds to seconds.
Attachment #8646732 - Flags: review?(jyavenard)
Attachment #8646731 - Flags: review?(jyavenard) → review+
Comment on attachment 8646731 [details]
MozReview Request: Bug 1193603. Part 1 - Fix indentation and include order. r=jya.

https://reviewboard.mozilla.org/r/15869/#review14115

Ship It!

::: dom/media/MediaMetadataManager.h:13
(Diff revision 1)
>  #include "nsAutoPtr.h"

Could order them in alphabetical order while at it.

with lowercase going first.

seems to be the common standard
Attachment #8646732 - Flags: review?(jyavenard)
Comment on attachment 8646732 [details]
MozReview Request: Bug 1193603. Part 2 - Fix miscalculation in converting micro seconds to seconds.

https://reviewboard.mozilla.org/r/15871/#review14117

::: dom/media/MediaMetadataManager.h:49
(Diff revision 1)
> -  void DispatchMetadataIfNeeded(AbstractMediaDecoder* aDecoder, double aCurrentTime) {
> +  void DispatchMetadataIfNeeded(AbstractMediaDecoder* aDecoder, int64_t aCurrentTime) {

please use TimeUnit objects instead. to prevent exactly those issues.
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Could order them in alphabetical order while at it.
> 
> with lowercase going first.
> 
> seems to be the common standard

Can you give some examples?
here that would give:
#include "mozilla/LinkedList.h"
#include "nsAutoPtr.h"
#include "AbstractMediaDecoder.h"
#include "VideoUtils.h"
I see. Thanks.
Comment on attachment 8646731 [details]
MozReview Request: Bug 1193603. Part 1 - Fix indentation and include order. r=jya.

Bug 1193603. Part 1 - Fix indentation and include order. r=jya.
Attachment #8646731 - Attachment description: MozReview Request: Bug 1193603. Part 1 - Fix indentation and include order. → MozReview Request: Bug 1193603. Part 1 - Fix indentation and include order. r=jya.
Comment on attachment 8646732 [details]
MozReview Request: Bug 1193603. Part 2 - Fix miscalculation in converting micro seconds to seconds.

Bug 1193603. Part 2 - Fix miscalculation in converting micro seconds to seconds.
Attachment #8646732 - Flags: review?(jyavenard)
Comment on attachment 8646732 [details]
MozReview Request: Bug 1193603. Part 2 - Fix miscalculation in converting micro seconds to seconds.

https://reviewboard.mozilla.org/r/15871/#review14119

::: dom/media/MediaDecoder.h:585
(Diff revision 2)
> -  void QueueMetadata(int64_t aPublishTime,
> +  void QueueMetadata(media::TimeUnit aPublishTime,

const media::TimeUnit& would be preferable

r=me with changes:
change prototyping with const media::TimeUnit& wherever possible (should be everwhere).
Attachment #8646732 - Flags: review?(jyavenard) → review+
Thanks for the review.
this needs to be uplifted to 41
https://hg.mozilla.org/mozilla-central/rev/6a1d0468f6d2
https://hg.mozilla.org/mozilla-central/rev/93e1af0f8be1
Assignee: nobody → jwwang
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> this needs to be uplifted to 41

MediaMetadataManager is used by chained ogg files only which are rarely useful. Is it worth the effort to uplift to 41?
You need to log in before you can comment on or make changes to this bug.