1193603 - Miscalculation in MediaMetadataManager::DispatchMetadataIfNeeded

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1193603. Part 1 - Fix indentation and include order. r=jya.

Bug 1193603. Part 1 - Fix indentation and include order.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 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.

Comment 3

[Jean-Yves Avenard [:jya]]

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

Comment 4

[Jean-Yves Avenard [:jya]]

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.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

(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?

Comment 6

[Jean-Yves Avenard [:jya]]

here that would give:
#include "mozilla/LinkedList.h"
#include "nsAutoPtr.h"
#include "AbstractMediaDecoder.h"
#include "VideoUtils.h"

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

I see. Thanks.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 10

[Jean-Yves Avenard [:jya]]

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).

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1d0468f6d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/93e1af0f8be1

Comment 13

[Jean-Yves Avenard [:jya]]

this needs to be uplifted to 41

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a1d0468f6d2
https://hg.mozilla.org/mozilla-central/rev/93e1af0f8be1

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(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?