Closed Bug 1360452 Opened 7 years ago Closed 7 years ago

Intermittent test_background_video_resume_after_end_show_last_frame.html | gizmo.{mp4-0,webm-2} video1 and video2 have different content.

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56

People

(Reporter: intermittent-bug-filer, Assigned: kaku)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:timing])

Attachments

(2 files)

disable test temporarily for windows/debug
1.19 KB, patch
kaku
: review+
Details | Diff | Splinter Review
Bug 1360452 - use SeekToNextFrame() to guarantee the last frame is shown on the reference video;
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Assignee: nobody → kaku
Summary: Intermittent dom/media/test/test_background_video_resume_after_end_show_last_frame.html | gizmo.mp4-0 video1 and video2 have different content. → Intermittent dom/media/test/test_background_video_resume_after_end_show_last_frame.html | gizmo.{mp4-0,webm-2} video1 and video2 have different content.
I'm having a semi permafail with changes from bug 1357040
Blocks: 1358057
:kaku, this is really failing very often on our integration trees, can you take a look at this sooner?  I would like to fix or disable this early next week- let me know if you need more time and we can disable it on win7 to ease the pain.
Flags: needinfo?(kaku)
Whiteboard: [stockwell needswork]
Still investigating, will disable it if I am not able to figure out the root cause.
Flags: needinfo?(kaku)
Comment on attachment 8865790 [details] [diff] [review]
disable test temporarily for windows/debug

Review of attachment 8865790 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for uploading this patch.

I think the root cause is that the playback of the "reference video" is not smooth, it drops frames. So, somehow, I should not produce the "reference" dynamically. I'll transform this test into a reference_test which will include a fix golden sample.
Attachment #8865790 - Flags: review?(kaku) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06d8db2020a
Intermittent dom/media/test/test_background_video_resume_after_end_show_last_frame.html. disable on win/debug. r=kaku
Keywords: leave-open
Whiteboard: [stockwell needswork] → [stockwell disabled]
Summary: Intermittent dom/media/test/test_background_video_resume_after_end_show_last_frame.html | gizmo.{mp4-0,webm-2} video1 and video2 have different content. → Intermittent test_background_video_resume_after_end_show_last_frame.html | gizmo.{mp4-0,webm-2} video1 and video2 have different content.
Depends on: 1365190
Comment on attachment 8868076 [details]
Bug 1360452 - use SeekToNextFrame() to guarantee the last frame is shown on the reference video;

https://reviewboard.mozilla.org/r/139682/#review143304
Attachment #8868076 - Flags: review?(jwwang) → review+
this will still likely fail due to SeekToNextFrame() being broken (see bug 1366362)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478

Still fail in the Win7 VM and the problem is here:
http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.cpp#1973-1975

Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be the last frame. 
I think we should instead seek to `mMaster->mDuraton`, :jw, is it reasonable?
Flags: needinfo?(jwwang)
(In reply to Tzuhao Kuo [:kaku] from comment #22)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> 
> Still fail in the Win7 VM and the problem is here:
> http://searchfox.org/mozilla-central/rev/
> a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> cpp#1973-1975
> 
> Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> the last frame. 
> I think we should instead seek to `mMaster->mDuraton`, :jw, is it reasonable?

An experiment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55f3b8a7ccc04af69bcda26e37bf07bf4c844d8
(In reply to Tzuhao Kuo [:kaku] from comment #22)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> 
> Still fail in the Win7 VM and the problem is here:
> http://searchfox.org/mozilla-central/rev/
> a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> cpp#1973-1975
> 
> Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> the last frame. 
Is it because NeedToSkipToNextKeyframe() returns true?
Flags: needinfo?(jwwang)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> (In reply to Tzuhao Kuo [:kaku] from comment #22)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> > 
> > Still fail in the Win7 VM and the problem is here:
> > http://searchfox.org/mozilla-central/rev/
> > a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> > cpp#1973-1975
> > 
> > Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> > the last frame. 
> Is it because NeedToSkipToNextKeyframe() returns true?

Not sure, let's try it by always returning false in the NeedToSkipToNextKeyframe().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6610d8f9450da58e92dd62534a3f8ea5f3e23ffe
(In reply to Tzuhao Kuo [:kaku] from comment #26)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> > (In reply to Tzuhao Kuo [:kaku] from comment #22)
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> > > 
> > > Still fail in the Win7 VM and the problem is here:
> > > http://searchfox.org/mozilla-central/rev/
> > > a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> > > cpp#1973-1975
> > > 
> > > Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> > > the last frame. 
> > Is it because NeedToSkipToNextKeyframe() returns true?
> 
> Not sure, let's try it by always returning false in the
> NeedToSkipToNextKeyframe().
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6610d8f9450da58e92dd62534a3f8ea5f3e23ffe

Let NeedToSkipToNextKeyframe() always return true doesn't help.
(In reply to Tzuhao Kuo [:kaku] from comment #27)
> (In reply to Tzuhao Kuo [:kaku] from comment #26)
> > (In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> > > (In reply to Tzuhao Kuo [:kaku] from comment #22)
> > > > https://treeherder.mozilla.org/#/
> > > > jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> > > > 
> > > > Still fail in the Win7 VM and the problem is here:
> > > > http://searchfox.org/mozilla-central/rev/
> > > > a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> > > > cpp#1973-1975
> > > > 
> > > > Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> > > > the last frame. 
> > > Is it because NeedToSkipToNextKeyframe() returns true?
> > 
> > Not sure, let's try it by always returning false in the
> > NeedToSkipToNextKeyframe().
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6610d8f9450da58e92dd62534a3f8ea5f3e23ffe
> 
> Let NeedToSkipToNextKeyframe() always return true doesn't help.

Sorry, it should be always return "false".
(In reply to Tzuhao Kuo [:kaku] from comment #26)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> > (In reply to Tzuhao Kuo [:kaku] from comment #22)
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> > > 
> > > Still fail in the Win7 VM and the problem is here:
> > > http://searchfox.org/mozilla-central/rev/
> > > a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> > > cpp#1973-1975
> > > 
> > > Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> > > the last frame. 
> > Is it because NeedToSkipToNextKeyframe() returns true?
> 
> Not sure, let's try it by always returning false in the
> NeedToSkipToNextKeyframe().
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6610d8f9450da58e92dd62534a3f8ea5f3e23ffe

So, making NeedToSkipToNextKeyframe() always returning false doesn't help. 
However, looks like the MFR doesn't honor the information from MDSM completely.
http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/dom/media/MediaFormatReader.cpp#1542-1556

Let's also make MediaFormatReader::ShouldSkip() always return false and try again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fa709cd9b217589a2fa33a62d6d853f3393f077
(In reply to Tzuhao Kuo [:kaku] from comment #30)
> (In reply to Tzuhao Kuo [:kaku] from comment #26)
> > (In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> > > (In reply to Tzuhao Kuo [:kaku] from comment #22)
> > > > https://treeherder.mozilla.org/#/
> > > > jobs?repo=try&revision=507053c1cebad7afa616c13ab36e8ad7eece2478
> > > > 
> > > > Still fail in the Win7 VM and the problem is here:
> > > > http://searchfox.org/mozilla-central/rev/
> > > > a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/MediaDecoderStateMachine.
> > > > cpp#1973-1975
> > > > 
> > > > Due to the slow machine, the `mMaster->mDecodedVideoEndTime` might not be
> > > > the last frame. 
> > > Is it because NeedToSkipToNextKeyframe() returns true?
> > 
> > Not sure, let's try it by always returning false in the
> > NeedToSkipToNextKeyframe().
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6610d8f9450da58e92dd62534a3f8ea5f3e23ffe
> 
> So, making NeedToSkipToNextKeyframe() always returning false doesn't help. 
> However, looks like the MFR doesn't honor the information from MDSM
> completely.
> http://searchfox.org/mozilla-central/rev/
> d441cb24482c2e5448accaf07379445059937080/dom/media/MediaFormatReader.
> cpp#1542-1556
> 
> Let's also make MediaFormatReader::ShouldSkip() always return false and try
> again.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3fa709cd9b217589a2fa33a62d6d853f3393f077

It works!
@JW, is your idea to have a pref for the skip-to-next-key-frame mechanism and turn off it while testing?
Flags: needinfo?(jwwang)
Yes. And I would like to remove MDSM::NeedToSkipToNextKeyframe() and keep MediaFormatReader::ShouldSkip() for we don't need 2 algorithms for frame-skipping. Does that make sense to you?
Flags: needinfo?(jwwang) → needinfo?(jyavenard)
It's a great idea, but everything will be in the detail...

The MFR makes a decision on data not available to the MDSM.

A gutfeel tells me that the MFR would be a better candidate to decide if it needs to skip or not.
Flags: needinfo?(jyavenard)
Hi Kaku,
Per comment 33, please open new bugs to:
1. remove NeedToSkipToNextKeyframe()
2. add a pref to enable/disable frame-skipping.

Thanks!
Depends on: 1371188
Finally, it works. Thanks for the review!
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c553c96bff0c
use SeekToNextFrame() to guarantee the last frame is shown on the reference video; r=jwwang
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [stockwell disabled] → [stockwell fixed:timing]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: