Closed Bug 1386478 Opened 6 years ago Closed 6 years ago

Wrong currentTime when playing a chained ogg file

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

chain.ogg
62.12 KB, video/ogg
Details
test_chaining_currenttime.html
458 bytes, text/html
Details
Bug 1386478 - don't cap the return value of GetEndTime().
59 bytes, text/x-review-board-request
jya
: review+
Details
Attached video chain.ogg — 
      No description provided.
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
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)
(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
See Also: → 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)
(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
Attachment #8892720 - Flags: review?(jyavenard)
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+
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.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d607b1cb6d58
don't cap the return value of GetEndTime(). r=jya
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
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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)
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.