Closed Bug 1427932 Opened 6 years ago Closed 6 years ago

endTime is wrong in OggDemuxer::SeekInternal()

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1427932. P1 - replace NS_ASSERTION with MOZ_ASSERT to catch more bugs.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1427932. P2 - fix the calculation of |endTime|.
59 bytes, text/x-review-board-request
jya
: review+
Details 
https://searchfox.org/mozilla-central/source/dom/media/ogg/OggDemuxer.cpp#1057

It should be

int64_t endTime = mInfo.mMetadataDuration->ToMicroseconds() + startTime;
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8939735 - Flags: review?(jyavenard)
Attachment #8939736 - Flags: review?(jyavenard)
Comment on attachment 8939735 [details]
Bug 1427932. P1 - replace NS_ASSERTION with MOZ_ASSERT to catch more bugs.

https://reviewboard.mozilla.org/r/210050/#review215696

not sure the comment is valid though, how could it catch more bugs?
Attachment #8939735 - Flags: review?(jyavenard) → review+
Comment on attachment 8939735 [details]
Bug 1427932. P1 - replace NS_ASSERTION with MOZ_ASSERT to catch more bugs.

https://reviewboard.mozilla.org/r/210050/#review215696

MOZ_ASSERT is more noticeable when it fails.
Comment on attachment 8939736 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/210052/#review215700

i dont think that's correct. mMetadataDuration is endTime already of metadata got retrieved, , it is only adjusted if the duration was retrieved by seeking to the end.

the issue is I believe deeper than this.
Attachment #8939736 - Flags: review?(jyavenard) → review-
Comment on attachment 8939736 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/210052/#review215700

https://searchfox.org/mozilla-central/source/dom/media/ogg/OggDemuxer.cpp#581
mInfo.mMetadataDuration.emplace(TimeUnit::FromMicroseconds(endTime - mStartTime.refOr(0)));

Since mMetadataDuration = endTime - mStartTime, we have endTime = mMetadataDuration + mStartTime, no?
Comment on attachment 8939736 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/210052/#review215700

yes, but as i mentioned, you only get there if no duration was found in the metadata,
https://searchfox.org/mozilla-central/source/dom/media/ogg/OggDemuxer.cpp#569

not having a duration and having to seek to the end would be rather uncommon I think.

or add a strong assert that startTime is always zero if a duration was found in the ogg metadata
Comment on attachment 8939736 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/210052/#review215700

I can't see how they are related. If mMetadataDuration is nothing at #569, it will call RangeEndTime() instead of SeekInternal() to determine the end time. SeekInternal() should be called only after mMetadataDuration is filled.
Comment on attachment 8939736 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/210052/#review215700

they could be related because if mMetadataDuration was determined using the Ogg's metadata (in the skeleton) and startTime isn't 0 then doing mInfo.mMetadataDuration->ToMicroseconds() + starTime will give you a wrong value as mInfo.mMetadataDuration is already the unadjusted endTime.

If mMetadataDuration wasn't determined from the Ogg's metadata, then it would be determined by seeking to the end of the track and looking at the time of the last sample: only then would mMetadataDuration be adjusted by startTime.

So what I suggested was either assume startTime is always 0 if mMetadataDuration was calculated from the Ogg's metadata, and if not then mMetadaDuration should be also adjusted (right now it isn't)...
Attachment #8939736 - Attachment is obsolete: true
The bug is to fix the assertion I hit when tweaking mochitests.

Repro steps:
1. apply P1 and P2.
2. build and run ./mach mochitest dom/media/test/test_seek-1.html --run-until-failure --repeat 10000
3. you will hit the assertion soon.

I can't really see how P2 makes a difference though.
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> they could be related because if mMetadataDuration was determined using the
> Ogg's metadata (in the skeleton) and startTime isn't 0 then doing
> mInfo.mMetadataDuration->ToMicroseconds() + starTime will give you a wrong
> value as mInfo.mMetadataDuration is already the unadjusted endTime.

It doesn't sound right to me. If mMetadataDuration is retrieved from the Oggs' metadata in the skeleton and skeleton isn't 0, MDSM will get the wrong duration from mMetadataDuration which is in fact the end time instead of the duration.

If we get mMetadataDuration from the skeleton, it should be adjusted by the startTime so we can always have mMetadataDuration == endTime - startTime no matter it is calculated from the skeleton or seeking to the end.
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> they could be related because if mMetadataDuration was determined using the
> Ogg's metadata (in the skeleton) and startTime isn't 0 then doing
> mInfo.mMetadataDuration->ToMicroseconds() + starTime will give you a wrong
> value as mInfo.mMetadataDuration is already the unadjusted endTime.

https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/media/ogg/OggCodecState.cpp#1743

It looks like the duration from the skeleton is already adjusted by the start time. So it is the true duration instead of the unadjusted endTime.
Flags: needinfo?(jyavenard)
ok
Flags: needinfo?(jyavenard)
Attachment #8940098 - Attachment is obsolete: true
Attachment #8941304 - Flags: review?(jyavenard)
Comment on attachment 8941304 [details]
Bug 1427932. P2 - fix the calculation of |endTime|.

https://reviewboard.mozilla.org/r/211600/#review217410
Attachment #8941304 - Flags: review?(jyavenard) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b7cc135f1a5
P1 - replace NS_ASSERTION with MOZ_ASSERT to catch more bugs. r=jya
https://hg.mozilla.org/integration/autoland/rev/fbae5269d99a
P2 - fix the calculation of |endTime|. r=jya
We need to fix bug 1429280 first.
Depends on: 1429280
Flags: needinfo?(jwwang)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5765be2f5f10
P1 - replace NS_ASSERTION with MOZ_ASSERT to catch more bugs. r=jya
https://hg.mozilla.org/integration/autoland/rev/b861f596cf9e
P2 - fix the calculation of |endTime|. r=jya
https://hg.mozilla.org/mozilla-central/rev/5765be2f5f10
https://hg.mozilla.org/mozilla-central/rev/b861f596cf9e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: