Wrong currentTime when playing a chained ogg file

RESOLVED FIXED in Firefox 57

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

3 months ago
Created attachment 8892714 [details]
chain.ogg
(Assignee)

Comment 1

3 months ago
Created attachment 8892715 [details]
test_chaining_currenttime.html

Add a test case to demonstrate the issue.

Chrome output:
event=timeupdate currentTime=0.200969
event=timeupdate currentTime=0.450871
event=timeupdate currentTime=0.700871
event=timeupdate currentTime=0.950829
event=timeupdate currentTime=1.200867
event=timeupdate currentTime=1.700648
event=timeupdate currentTime=1.950777
event=timeupdate currentTime=2.45066
event=timeupdate currentTime=2.700752
event=timeupdate currentTime=3.200813
event=timeupdate currentTime=3.700889
event=timeupdate currentTime=4.256145
event=timeupdate currentTime=4.256145
event=ended currentTime=4.256145

Firefox output:
event=timeupdate currentTime=0.023469
event=timeupdate currentTime=0.281337
event=timeupdate currentTime=0.56238
event=timeupdate currentTime=0.844603
event=timeupdate currentTime=1
event=timeupdate currentTime=Infinity
event=timeupdate currentTime=Infinity
event=timeupdate currentTime=Infinity
event=timeupdate currentTime=Infinity
event=timeupdate currentTime=Infinity
event=ended currentTime=Infinity

The currentTime should go from 0 to above 4 as shown in Chrome.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 months ago
http://searchfox.org/mozilla-central/diff/fa7bd08dd26cd392097001fa24b245d849f5bb20/dom/media/mediasink/DecodedAudioDataSink.cpp#205

This is a regression from Bug 1264199 P1 which caps the end time to the end time of the last audio sample. However, this is not appropriate for a chained ogg file since it contains multiple streams and timestamps will not monolithically increase.

Hi jya,
As the comment said, what is your concern if GetEndTime() returns a value greater than the original end time of the audio samples? Since rounding errors are always there when you convert frame count to microseconds even without resampling, I don't feel the need to cap the return value of GetEndTime().
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

3 months ago
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> Created attachment 8892720 [details]
> Bug 1386478 - don't cap the return value of GetEndTime().
> 
> A chained ogg file contains multiple streams and timestamps will not
> monolithically increase. The current code will cap AudioEndTime()
> to the end time of the 1st stream and currentTime won't be updated
> correctly when playback position reaches the 2nd stream or later ones.
> 
> http://searchfox.org/mozilla-central/rev/
> bbc1c59e460a27b20929b56489e2e55438de81fa/dom/media/MediaDecoderStateMachine.
> cpp#3636-3637
> 
> Review commit: https://reviewboard.mozilla.org/r/163712/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/163712/

With this patch, Firefox outputs:
event=timeupdate currentTime=0.02229
event=timeupdate currentTime=0.280408
event=timeupdate currentTime=0.564285
event=timeupdate currentTime=0.846916
event=timeupdate currentTime=1.008911
event=timeupdate currentTime=1.293378
event=timeupdate currentTime=1.576893
event=timeupdate currentTime=1.859591
event=timeupdate currentTime=2.020952
event=timeupdate currentTime=2.303446
event=timeupdate currentTime=2.586099
event=timeupdate currentTime=2.868072
event=timeupdate currentTime=3.03009
event=timeupdate currentTime=3.312086
event=timeupdate currentTime=3.594739
event=timeupdate currentTime=3.877256
event=timeupdate currentTime=Infinity
event=timeupdate currentTime=Infinity
event=ended currentTime=Infinity

Note we still have other issues about duration/currentTime (bug 1385699) to fix. But we are on the right track to make currentTime updated correctly.
Blocks: 1385699
Priority: -- → P3
(Assignee)

Updated

3 months ago
See Also: → bug 1264199
I don't recall that change.

Likely to pass some mochitests, or that the duration reported could never be different to what has been demuxed/decoded

It didn't make sense to me that the duration could be set to how something is *played*
Flags: needinfo?(jyavenard)
(Assignee)

Comment 6

3 months ago
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> It didn't make sense to me that the duration could be set to how something
> is *played*
Not sure if I follow. It is the end time instead of the duration.

http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/dom/media/mediasink/MediaSink.h#62
MediaSink::GetEndTime() returns the end time of the data that has been consumed. It is used to cap the system clock so MDSM will not advance playback position beyond the end of media.

http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/dom/media/MediaDecoderStateMachine.cpp#3623-3625
(Assignee)

Updated

3 months ago
Attachment #8892720 - Flags: review?(jyavenard)

Comment 7

3 months ago
mozreview-review
Comment on attachment 8892720 [details]
Bug 1386478 - don't cap the return value of GetEndTime().

https://reviewboard.mozilla.org/r/163712/#review169618

::: dom/media/mediasink/AudioSink.cpp:236
(Diff revision 1)
>    TimeUnit played = FramesToTimeUnit(written, mOutputRate) + mStartTime;
>    if (!played.IsValid()) {
>      NS_WARNING("Int overflow calculating audio end time");
>      return TimeUnit::Zero();
>    }
> -  // As we may be resampling, rounding errors may occur. Ensure we never get
> +  return played;

wouldnt it be better to adjust mLastEndTime instead whenever we go to another ogg file?

That would become mLastEndTime += newEndTime
Attachment #8892720 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 8

3 months ago
mozreview-review-reply
Comment on attachment 8892720 [details]
Bug 1386478 - don't cap the return value of GetEndTime().

https://reviewboard.mozilla.org/r/163712/#review169618

> wouldnt it be better to adjust mLastEndTime instead whenever we go to another ogg file?
> 
> That would become mLastEndTime += newEndTime

It would be complicated to do that because AudioSink has no knowledage about ogg. It might be easier to have OggDemuxer adjust the timestampes of the demuxed samples.

Comment 9

3 months ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d607b1cb6d58
don't cap the return value of GetEndTime(). r=jya

Comment 10

3 months ago
mozreview-review-reply
Comment on attachment 8892720 [details]
Bug 1386478 - don't cap the return value of GetEndTime().

https://reviewboard.mozilla.org/r/163712/#review169618

> It would be complicated to do that because AudioSink has no knowledage about ogg. It might be easier to have OggDemuxer adjust the timestampes of the demuxed samples.

that seems like a better approach then.
right now, the end time of a sample being played can change depending on the hardware on which it plays (as we resample)
https://hg.mozilla.org/mozilla-central/rev/d607b1cb6d58
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

3 months ago
Blocks: 1387310
Depends on: 1398139
Well, seems to me we should revert that bug, and do the ogg fix properly as discussed in comment 7.

we simply need to adjust currentTime/EndTime as the information about new playback is gathered.
Flags: needinfo?(jwwang)
(Assignee)

Comment 13

a month ago
http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/dom/media/mediasink/AudioSink.cpp#449

Yes, we should fix ogg so that the end time of each audio sample from a chained ogg is correct and mono-increasing.
Flags: needinfo?(jwwang)
You need to log in before you can comment on or make changes to this bug.