Closed Bug 1414444 Opened 7 years ago Closed 1 year ago

Assertion failure: aTime >= 0.0 (Cannot seek to a negative value.) [@ mozilla::MediaDecoder::Seek]

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
58 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
120 Branch
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox-esr78 --- wontfix
firefox-esr102 --- wontfix
firefox-esr115 --- wontfix
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox82 --- wontfix
firefox83 --- wontfix
firefox84 --- wontfix
firefox85 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- wontfix
firefox118 --- wontfix
firefox119 --- wontfix
firefox120 --- fixed

People

(Reporter: tsmith, Assigned: karlt)

References

(Blocks 1 open bug, Regressed 1 open bug, Regression)

Details

(Keywords: assertion, regression, testcase)

Attachments

(5 files)

testcase.mp4
33.21 KB, video/mp4
Details
Bug 1414444 - add moof only when it is valid.
59 bytes, text/x-review-board-request
jya
: review-
Details
Bug 1414444 skip an invalid complete moof r?kinetik
48 bytes, text/x-phabricator-request
Details | Review
Bug 1414444 add testcase as crashtest r?kinetik
48 bytes, text/x-phabricator-request
Details | Review
Bug 1414444 include existing Sample count in Moof::mIndex.SetCapacity() r?kinetik
48 bytes, text/x-phabricator-request
Details | Review
Attached video testcase.mp4 —
Assertion failure: aTime >= 0.0 (Cannot seek to a negative value.), at /src/dom/media/MediaDecoder.cpp:693 #0 mozilla::MediaDecoder::Seek(double, mozilla::SeekTarget::Type) /src/dom/media/MediaDecoder.cpp:690:3 #1 mozilla::MediaDecoder::DurationChanged() /src/dom/media/MediaDecoder.cpp:1075:5 #2 mozilla::ChannelMediaDecoder::DurationChanged() /src/dom/media/ChannelMediaDecoder.cpp:396:17 #3 mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher::DoNotify() /src/obj-firefox/dist/include/mozilla/StateWatching.h:279:9 #4 mozilla::detail::RunnableMethodImpl<mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher*, void (mozilla::WatchManager<mozilla::MediaDecoder>::PerCallbackWatcher::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13 #5 mozilla::AutoTaskDispatcher::DrainDirectTasks() /src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:105:10 #6 mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() /src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:206:9 #7 mozilla::EventTargetWrapper::Runner::Run() /src/xpcom/threads/AbstractThread.cpp:154:32 #8 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1037:14 #9 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:513:10 #10 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21 #11 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10 #12 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3 #13 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27 #14 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30 #15 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4675:22 #16 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4837:8 #17 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4932:21 #18 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22 #19 main /src/browser/app/nsBrowserApp.cpp:304:16 #20 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291 #21 _start (firefox+0x41ebe4)
Flags: in-testsuite?
INFO: Last good revision: aecaa85bb1d9d8b7d2cbd9ab84eb6dbc0ed9eee8 INFO: First bad revision: 4e892b27c2f112e61a74d1bd75cc1d73ff91e2b2 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aecaa85bb1d9d8b7d2cbd9ab84eb6dbc0ed9eee8&tochange=4e892b27c2f112e61a74d1bd75cc1d73ff91e2b2
Blocks: 1387798
Has Regression Range: --- → yes
status-firefox56: --- → unaffected
status-firefox57: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(ayang)
Invalid 'trun' table.
Assignee: nobody → ayang
Flags: needinfo?(ayang)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1) > INFO: Last good revision: aecaa85bb1d9d8b7d2cbd9ab84eb6dbc0ed9eee8 > INFO: First bad revision: 4e892b27c2f112e61a74d1bd75cc1d73ff91e2b2 > INFO: Pushlog: > https://hg.mozilla.org/integration/autoland/ > pushloghtml?fromchange=aecaa85bb1d9d8b7d2cbd9ab84eb6dbc0ed9eee8&tochange=4e89 > 2b27c2f112e61a74d1bd75cc1d73ff91e2b2 It's because ByteReader doesn't return any error before that changeset. It just uses zero when failing to read anything.
Incorrect 'trun' flag '0xFFFF' causes this problem.
Comment on attachment 8926219 [details] Bug 1414444 - add moof only when it is valid. https://reviewboard.mozilla.org/r/197474/#review202764 ::: media/libstagefright/binding/MoofParser.cpp:67 (Diff revision 1) > mInitRange = MediaByteRange(0, box.Range().mEnd); > ParseMoov(box); > } else if (box.IsType("moof")) { > Moof moof(box, mTrex, mMvhd, mMdhd, mEdts, mSinf, &mLastDecodeTime, mIsAudio); > > if (!moof.IsValid() && !box.Next().IsAvailable()) { change the test here if it isnt valid. and if it's complete, but invalid, somehow it would be nice to have the error reported than just ignoree.
Attachment #8926219 - Flags: review?(jyavenard) → review-
Severity: normal → S3

https://pernos.co/debug/r5p0WBn7Te_YkAzjztZfbQ/index.html is a recording with more recent source code.

Moof::ParseTrun() appends samples to the Moof but does not update the decode time when the function returns early. This contributes to subsequent samples having earlier presentation times than those in the invalid moof.

Discarding the invalid Moof, as in attachment 8926219 [details], would seem a reasonable approach to address that. If doing so, the current inconsistent approach to decode time advancing for discarded successfully-parsed truns in the same Moof could also do with addressing.

An alternative approach of advancing the decode time for Samples successfully parsed from the invalid trun might be better for A/V sync, but there would be a gap in the buffered ranges. mTimeRange is already not set on an invalid moof. Gap-filling between Samples is also is not performed, so playback of the samples could be problematic.

I assume we don't need to aim to play back invalid mp4 well. A solution here is more about making MoofParser return something that doesn't break invariants Gecko is expecting and freeing up fuzzing to find undiagnosed problems.

The testcase also exposes bug 1850851, but that did not exist when this bug was filed.

See Also: → 1850851

(In reply to Jean-Yves Avenard [:jya] from comment #6)

   if (!moof.IsValid() && !box.Next().IsAvailable()) {

change the test here if it isnt valid.

This was a review on https://hg.mozilla.org/mozreview/gecko/rev/43ff74d5758a29989f3f62fb015f1dcb4535ac70.
Handling an invalid Moof as suggested here skips unnecessary FixRounding().

Care needs to be taken to not advance mOffset if the Moof is not complete.
https://hg.mozilla.org/mozreview/gecko/rev/4d81c5f8293dabb5a69d8e1594414beb1bf626a7

and if it's complete, but invalid, somehow it would be nice to have the
error reported than just ignoree.

There's an existing LOG_WARN(), but MoofParser doesn't otherwise attempt to report the nature of errors to its clients. Doing so is beyond the scope of this bug.

and don't advance decode time for complete truns in the incomple moof, the
samples of which already did not contribute to mTimeRange and were not
gap-filled.

Assignee: nobody → karlt
Status: NEW → ASSIGNED

Testcase from Tyson Smith <[email protected]>

Depends on D187170

so that AppendElement() always succeeds, as expected.

A single Moof can parse multiple truns.

The comment is removed because I'd prefer to keep mIndex as a FallibleTArray
to remind anyone changing the code to keep allocations fallible.

Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/886a64cfbbf7 include existing Sample count in Moof::mIndex.SetCapacity() r=kinetik

https://hg.mozilla.org/mozilla-central/rev/9f3d02701788
https://hg.mozilla.org/mozilla-central/rev/886a64cfbbf7

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox120: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch
Regressions: 1855368
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: