Closed Bug 1409946 Opened 7 years ago Closed 6 years ago

Figure out why the crashtest from bug 1389304 frequently asserts in CI

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: RyanVM, Assigned: alwu)

References

Details

Attachments

(2 files)

I attempted to create a crashtest from the testcase attached to bug 1389304. However, it frequently hits assertions that make it orange when I run it through Try (mostly on OSX, but Linux and Windows are also affected).

Here's the patch:
https://hg.mozilla.org/try/rev/7b871da4b8d8a6a88d6c00b46c8026315d354016

And the resulting failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=137994305&repo=try

[Child 742, MediaPDecoder #1] ###!!! ASSERTION: mQueuedSample must be a keyframe: 'mQueuedSample->mKeyframe', file /builds/worker/workspace/build/src/dom/media/fmp4/MP4Demuxer.cpp, line 496
#01: mozilla::detail::ProxyFunctionRunnable<mozilla::MediaFormatReader::DemuxerProxy::Wrapper::GetSamples(int)::'lambda'(), mozilla::MozPromise<RefPtr<mozilla::MediaTrackDemuxer::SamplesHolder>, mozilla::MediaResult, true> >::Run() [mfbt/UniquePtr.h:340]
#02: mozilla::TaskQueue::Runner::Run() [xpcom/threads/TaskQueue.h:175]
#03: nsThreadPool::Run() [xpcom/base/nsCOMPtr.h:424]
#04: non-virtual thunk to nsThreadPool::Run() [xpcom/threads/nsThreadPool.cpp:156]
#05: nsThread::ProcessNextEvent(bool, bool*) [mfbt/Maybe.h:224]
#06: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:512]
#07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:364]
#08: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:599]
#09: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:427]
#10: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:219]
#11: libsystem_pthread.dylib + 0x405a
#12: libsystem_pthread.dylib + 0x3fd7
Is this the same key frame issue you see before?
Flags: needinfo?(kikuo)
Sadly, I don't remember I've seen this issue before.

Apparently, the mQueueSample is not obtained from SkipToNextRandomAccessPoint(), should be related to here [1]

[1] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/fmp4/MP4Demuxer.cpp#439-449
Flags: needinfo?(kikuo)
ni Alastor who has been working on MSE and MFR for a while.
Flags: needinfo?(alwu)
Attached file Warning log
I'm wondering why we support this kind of video [1], it seems has not been muxed well.

This video can't be playback well, it only shows the single frame and its current time won't be updated. I can also see the wrong current time and broken image when start playing it. In addition, this video can't be playback on Chrome or VLC.

---

Hi, Alfredo,
Is correct to support this video?
Thanks!

[1] https://bug1389304.bmoattachments.org/attachment.cgi?id=8896046
Flags: needinfo?(alwu) → needinfo?(ayang)
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #4)
> Created attachment 8920523 [details]
> Warning log
> 
> I'm wondering why we support this kind of video [1], it seems has not been
> muxed well.
> 
> This video can't be playback well, it only shows the single frame and its
> current time won't be updated. I can also see the wrong current time and
> broken image when start playing it. In addition, this video can't be
> playback on Chrome or VLC.
> 
> ---
> 
> Hi, Alfredo,
> Is correct to support this video?
> Thanks!
> 
> [1] https://bug1389304.bmoattachments.org/attachment.cgi?id=8896046

I can't repdouce it on my local. Is it 100% reproducible?

And yes, this file is buggy, its has an incorrect table that audio sample can't be demuxed correctly. But if this is content problem, I think it'd be 100% reproducible. And the assertion is on video samples, not audio.
Flags: needinfo?(ayang)
FYI, 
A strange found is if you play the video on local file, the file duration and current time would be correct, but if you play the video via URL, the current time would be wrong.
Depends on: 1389304
No longer depends on: 1388304
Hi, Ryan,
I couldn't reproduce this issue [1] on try server, it seems that it has low reproduce rate.
Maybe you could consider to land it now?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ea110815bf59049da9116ea761d2ea221d0a46c&selectedJob=140634848
Flags: needinfo?(ryanvm)
I'm confused, the Try run you're linking to is showing exactly the failure this bug was filed for happening ~50% of the time on OSX?
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> I'm confused, the Try run you're linking to is showing exactly the failure
> this bug was filed for happening ~50% of the time on OSX?

Ah, sorry, my bad.
I misread your comment, I thought that the fail test would be 1389304.html instead of video-replay-after-audio-end.html.
Assignee: nobody → alwu
Hi, jya,
Since we can't make sure the sample we got is always a key frame [1], do we still need this assertion? [2]
Thanks!
Eg. you can reproduce this issue easily by seek the *local* file [3]  (if you play it via URL, you won't get the available seekable object)

[1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/media/fmp4/MP4Demuxer.cpp#413
[2] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/media/fmp4/MP4Demuxer.cpp#495
[3] *download this file* 
https://bug1389304.bmoattachments.org/attachment.cgi?id=8896046
Flags: needinfo?(jyavenard)
If you remove the assertion, then if mQueuedFrame isn't a keyframe you need to fall back to the standard code (which will search for the next keyframe)
Flags: needinfo?(jyavenard)
Comment on attachment 8925464 [details]
Bug 1409946 - ensure we could always get keyframe while seeking.

https://reviewboard.mozilla.org/r/196562/#review201818

::: dom/media/fmp4/MP4Demuxer.cpp:413
(Diff revision 1)
>    mQueuedSample = nullptr;
>  
>    mIterator->Seek(seekTime.ToMicroseconds());
>  
>    // Check what time we actually seeked to.
> -  mQueuedSample = GetNextSample();
> +  RefPtr<MediaRawData> sample;

remove (see below)

::: dom/media/fmp4/MP4Demuxer.cpp:415
(Diff revision 1)
>    mIterator->Seek(seekTime.ToMicroseconds());
>  
>    // Check what time we actually seeked to.
> -  mQueuedSample = GetNextSample();
> -  if (mQueuedSample) {
> +  RefPtr<MediaRawData> sample;
> +  do {
> +    sample = GetNextSample();

RefPtr<MediaRawData> sample = ...

keep the definition local to the do { } while

::: dom/media/fmp4/MP4Demuxer.cpp
(Diff revision 1)
>      return SamplesPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_DEMUXER_ERR,
>                                             __func__);
>    }
>  
>    if (mQueuedSample) {
> -    NS_ASSERTION(mQueuedSample->mKeyframe,

you definitely don't want to remove the assertion here, seeing that you're change above made it guarantee that mQueueSample is a keyframe
Attachment #8925464 - Flags: review?(jyavenard) → review+
Comment on attachment 8925464 [details]
Bug 1409946 - ensure we could always get keyframe while seeking.

https://reviewboard.mozilla.org/r/196562/#review201820

::: commit-message-d3910:1
(Diff revision 1)
> +Bug 1409946 - remove assertion and ensure we could always get keyframe while seeking.

change the commit description.

The first part of this change is to ensure mQueuedSample is only ever set to a keyframe.

So why remove the assertion?
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55c1cb6938fc
ensure we could always get keyframe while seeking. r=jya
https://hg.mozilla.org/mozilla-central/rev/55c1cb6938fc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.