1114805 - [regression] MP4 playback cause assert MOZ_ASSERT(startTime >= 0);

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This is a recent regression.

first pts here is -21333 ; which causes the assert.

There's nothing that states the first timestamp should be 0 in a mp4 container.

(lldb) bt
* thread #83: tid = 0x1cd0c, 0x00000001060bc5fc XUL`mozilla::MediaDecoderReader::ComputeStartTime(this=0x0000000120110000, aVideo=0x000000011e484580, aAudio=0x0000000129df3470) + 508 at MediaDecoderReader.cpp:180, name = 'Media Decode #1', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001060bc5fc XUL`mozilla::MediaDecoderReader::ComputeStartTime(this=0x0000000120110000, aVideo=0x000000011e484580, aAudio=0x0000000129df3470) + 508 at MediaDecoderReader.cpp:180
    frame #1: 0x00000001060c33c7 XUL`mozilla::MediaDecoderStateMachine::FinishDecodeFirstFrame(this=0x000000011f9be000) + 359 at MediaDecoderStateMachine.cpp:2201
    frame #2: 0x00000001060c1ceb XUL`mozilla::MediaDecoderStateMachine::MaybeFinishDecodeFirstFrame(this=0x000000011f9be000) + 139 at MediaDecoderStateMachine.cpp:950
    frame #3: 0x00000001060c1674 XUL`mozilla::MediaDecoderStateMachine::OnAudioDecoded(this=0x000000011f9be000, aAudioSample=0x0000000129df3470) + 644 at MediaDecoderStateMachine.cpp:757
    frame #4: 0x00000001061173b7 XUL`void mozilla::MediaPromise<nsRefPtr<mozilla::AudioData>, mozilla::MediaDecoderReader::NotDecodedReason>::InvokeCallbackMethod<mozilla::MediaDecoderStateMachine, mozilla::AudioData*>(aThisVal=0x000000011f9be000, aMethod=0x00000001060c13f0, aValue=0x0000000129df3470)(mozilla::AudioData*), mozilla::MediaPromise<nsRefPtr<mozilla::AudioData>, mozilla::MediaDecoderReader::NotDecodedReason>::NonDeduced<mozilla::AudioData*>::type) + 135 at MediaPromise.h:184
    frame #5: 0x000000010611722b XUL`mozilla::MediaPromise<nsRefPtr<mozilla::AudioData>, mozilla::MediaDecoderReader::NotDecodedReason>::ThenValue<mozilla::MediaTaskQueue, mozilla::MediaDecoderStateMachine, void (this=0x000000011ff3d040, aResolveValue=mozilla::MediaPromise<nsRefPtr<mozilla::AudioData>, mozilla::MediaDecoderReader::NotDecodedReason>::ResolveValueType at 0x0000000130489850)(mozilla::AudioData*), void (mozilla::MediaDecoderStateMachine::*)(mozilla::MediaDecoderReader::NotDecodedReason)>::DoResolve(nsRefPtr<mozilla::AudioData>) + 107 at MediaPromise.h:225
    frame #6: 0x00000001061179fc XUL`mozilla::MediaPromise<nsRefPtr<mozilla::AudioData>, mozilla::MediaDecoderReader::NotDecodedReason>::ThenValueBase::ResolveRunnable::Run(this=0x000000012accca30) + 220 at MediaPromise.h:111
    frame #7: 0x00000001060ecf19 XUL`mozilla::MediaTaskQueue::Runner::Run(this=0x00000001201da600) + 633 at MediaTaskQueue.cpp:230
    frame #8: 0x0000000103705107 XUL`nsThreadPool::Run(this=0x000000012aad3c90) + 967 at nsThreadPool.cpp:225
    frame #9: 0x00000001037051fc XUL`non-virtual thunk to nsThreadPool::Run(this=0x000000012aad3c98) + 28 at nsThreadPool.cpp:239
    frame #10: 0x0000000103701b68 XUL`nsThread::ProcessNextEvent(this=0x000000011e7512f0, aMayWait=false, aResult=0x0000000130489c4e) + 2088 at nsThread.cpp:855
    frame #11: 0x000000010375add7 XUL`NS_ProcessNextEvent(aThread=0x000000011e7512f0, aMayWait=false) + 151 at nsThreadUtils.cpp:265
    frame #12: 0x0000000103d852f3 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x000000011cec4c00, aDelegate=0x000000012a2bd940) + 691 at MessagePump.cpp:339
    frame #13: 0x0000000103d1c775 XUL`MessageLoop::RunInternal(this=0x000000012a2bd940) + 117 at message_loop.cc:233
    frame #14: 0x0000000103d1c685 XUL`MessageLoop::RunHandler(this=0x000000012a2bd940) + 21 at message_loop.cc:226
    frame #15: 0x0000000103d1c62d XUL`MessageLoop::Run(this=0x000000012a2bd940) + 45 at message_loop.cc:200
    frame #16: 0x00000001036ffff6 XUL`nsThread::ThreadFunc(aArg=0x000000011e7512f0) + 358 at nsThread.cpp:356
    frame #17: 0x0000000103371cff libnss3.dylib`_pt_root(arg=0x000000012e0a7840) + 463 at ptthread.c:212
    frame #18: 0x00007fff8ea2b2fc libsystem_pthread.dylib`_pthread_body + 131
    frame #19: 0x00007fff8ea2b279 libsystem_pthread.dylib`_pthread_start + 176
    frame #20: 0x00007fff8ea294b1 libsystem_pthread.dylib`thread_start + 13
(lldb)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Don't use hard assertion should start time be <= 0

Make it a soft-assert. There's no requirement that a file has a positive start time.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

When playing the testcase in Windows, WMF's AAC decoder also outputs the first sample with a timestamp of -21333, but in WMFAudioMFTManager::Output(), I strip off the negatively timstamped samples to that the audio stream begins at time 0. I assumed that the samples I was stripping were preroll or somesuch. Should we not be stripping the samples on Windows?

Comment 3

[Jean-Yves Avenard [:jya]]

You mentioned to me about that pre-roll stuff, tbh, I didn't quite get it..

Seems to me more of an issue with the muxed file than anything else.

So yes, I think this should be removed.

Do you have a sample? (maybe you too used that facebook video example, seeing that I got it from your website :) )

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> You mentioned to me about that pre-roll stuff, tbh, I didn't quite get it..

Neither did I, and I didn't know anyone who had a clue about AAC, so I just did what worked.


> Seems to me more of an issue with the muxed file than anything else.
> 
> So yes, I think this should be removed.

Just so we're clear, do you think that the negative timestamp is a valid decoding of the AAC stream, so the pruning of negative timestamped samples in the WMF PDM should be removed?

Or do you only think the assertion should be non-fatal?

We should have the same behaviour between Windows and MacOSX platforms here I think. Both decoders are outputting negatively timestamped first samples, so I'm inclined to believe it's not a bug in the WMF backed for it to do that.
 
> Do you have a sample? (maybe you too used that facebook video example,
> seeing that I got it from your website :) )

On http://people.mozilla.org/~jyavenard/tests/mse_mp4/facebook-paper.mp4 the WMF backend outputs the first AAC packet with a timestamp of -21333. So yes, the facebook-paper video. I think I've seen it on other videos, but I don't recall where.

Comment 5

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42dd185fb447

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

ni? on the questions in comment 4.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > You mentioned to me about that pre-roll stuff, tbh, I didn't quite get it..
> 
> Neither did I, and I didn't know anyone who had a clue about AAC, so I just
> did what worked.
> 
> 
> > Seems to me more of an issue with the muxed file than anything else.
> > 
> > So yes, I think this should be removed.
> 
> Just so we're clear, do you think that the negative timestamp is a valid
> decoding of the AAC stream, so the pruning of negative timestamped samples
> in the WMF PDM should be removed?
> 
> Or do you only think the assertion should be non-fatal?

I think making a soft assert for start time allows us to handle those samples where only the start is slightly negative and the rest is fine.
MSE will massage segment close enough to 0 to make them 0 (by shifting all timestamp) (bug 1129732). Currently it only does so for positive time. Should make them for samples starting < 0 too

The only issue I can foresee is that it is theoretically possible to have something with all negative timestamps; should we try to seek in there we would hard assert as the position is negative.

the MDSM should stop using -1 start and end values to mean non-initialised. and handle negative ranges properly.
Alternatively, could do something similar to what I did with MSE, store an offset calculated from the first sample and shift all samples so they are within a [0-X] range.

That the windows decoder drops a sample or two at the start won't be noticeable. As it is removing that assert can't introduce worth behaviour

> 
> We should have the same behaviour between Windows and MacOSX platforms here
> I think. Both decoders are outputting negatively timestamped first samples,
> so I'm inclined to believe it's not a bug in the WMF backed for it to do
> that.

it is not now.
The timestamps are contained in the mp4 container, not within the AAC sample

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42dd185fb447

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #4)
> > Or do you only think the assertion should be non-fatal?
> 
> I think making a soft assert for start time allows us to handle those
> samples where only the start is slightly negative and the rest is fine.
> MSE will massage segment close enough to 0 to make them 0 (by shifting all
> timestamp) (bug 1129732). Currently it only does so for positive time.
> Should make them for samples starting < 0 too
> 
> The only issue I can foresee is that it is theoretically possible to have
> something with all negative timestamps; should we try to seek in there we
> would hard assert as the position is negative.
 

> the MDSM should stop using -1 start and end values to mean non-initialised.
> and handle negative ranges properly.

I agree, we should use Optional<> or a similar construct instead of a sentinel value to indicate being not initialized.

> Alternatively, could do something similar to what I did with MSE, store an
> offset calculated from the first sample and shift all samples so they are
> within a [0-X] range.

What about non-MSE video?

It seems to me that we should make the MDSM handle negative timestamps, and not strip the negative timestamps in the WMF PDM.

> That the windows decoder drops a sample or two at the start won't be
> noticeable. As it is removing that assert can't introduce worth behaviour
> 
> > 
> > We should have the same behaviour between Windows and MacOSX platforms here
> > I think. Both decoders are outputting negatively timestamped first samples,
> > so I'm inclined to believe it's not a bug in the WMF backed for it to do
> > that.
> 
> it is not now.
> The timestamps are contained in the mp4 container, not within the AAC sample

OK. We can leave the assert in, but we should follow up to remove the code to strip negative timestamps from the WMF PDM (so we have the same cross platform behaviour) and to make the MDSM handle negative timestamps. Please file bugs for these.