Closed Bug 1331070 Opened 3 years ago Closed 3 years ago

Intermittent dom/media/test/test_seek-4.html | Test timed out.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

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

References

Details

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

Attachments

(2 files)

We're getting timeouts in pretty much all the seek tests recently...
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(jyavenard)
Flags: needinfo?(gsquelart)
Flags: needinfo?(jyavenard)
Looks the same as bug 1321198 (a timeout in test_seek-4.html) but with a different message.
I hesitate to make this bug a duplicate, so we don't lose the associated OrangeFactor data; so I'm linking them through "See Also".

JW is looking into it, see bug 1321198 comment 17.
Flags: needinfo?(gsquelart)
See Also: → 1321198
any further updates here?  I see bug 1321198 having no updates in the last ~10 days.  Is there any information needed to help clarify this bug?
See Also: → 1331281, 1330880
no action in bug 1321198, I have pinged for help there, but if we don't get any traction in the next week or two I will recommend disabling affected tests.
Duplicate of this bug: 1321198
Duplicate of this bug: 1331281
Duplicate of this bug: 1330880
Duplicate of this bug: 1337966
Attachment #8835231 - Flags: review?(kaku)
Comment on attachment 8835231 [details]
Bug 1331070 - delay seek request until decoding first frames for non-MSE media.

https://reviewboard.mozilla.org/r/110920/#review112226

No problem to the code modification.

But, the treeherder log in bug 1321198 comment 17 is missing, so I am not able to figure out the root cause. Is this patch an attempt? Or it is the right fix?

::: dom/media/MediaDecoderStateMachine.cpp:604
(Diff revision 1)
> +    if (mMaster->mIsMSE) {
> +      return StateObject::HandleSeek(aTarget);
> +    }
> +    // Delay seek request until decoding first frames for non-MSE media.
> +    SLOG("Not Enough Data to seek at this stage, queuing seek");
> +    mPendingSeek.RejectIfExists(__func__);
> +    mPendingSeek.mTarget.emplace(aTarget);
> +    return mPendingSeek.mPromise.Ensure(__func__);

This is not really an issue, just some ideas.

Here we have two different logics for handling seek. Totally, we have three kinds over all states:
(1) reject the seek: used by DecodeMetaData, ShutDownState.
(2) postpone the seek: used by WaitForCDM and non-MSE DecodingFirstFrame.
(3) execute the seek: used by others.

How about having a new class, say SeekHandler, with three kinds of inheritances and we compose the right SeekHandler into SeekObjects. By this way, we can avoid the if-statement here.
Attachment #8835231 - Flags: review?(kaku) → review+
(In reply to Tzuhao Kuo [:kaku] from comment #17)
> But, the treeherder log in bug 1321198 comment 17 is missing, so I am not
> able to figure out the root cause. Is this patch an attempt? Or it is the
> right fix?

The issue is if we try to seek on a non-MSE stream while decoding first frames, sometimes we get decode errors on win8 x64 opt. The patch is a workaround to delay processing the seek request until decoding first frames is done. Note somehow this issue seems to happen on win8 x64 opt. I think this is probably a problem of the windows decoder.
(In reply to Tzuhao Kuo [:kaku] from comment #17)
> Here we have two different logics for handling seek. Totally, we have three
> kinds over all states:
> (1) reject the seek: used by DecodeMetaData, ShutDownState.
> (2) postpone the seek: used by WaitForCDM and non-MSE DecodingFirstFrame.
> (3) execute the seek: used by others.
> 
> How about having a new class, say SeekHandler, with three kinds of
> inheritances and we compose the right SeekHandler into SeekObjects. By this
> way, we can avoid the if-statement here.

Can you open a new bug and have a WIP to illustrate the idea?
Thanks for the review!
Assignee: nobody → jwwang
Priority: -- → P1
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e059386f261
delay seek request until decoding first frames for non-MSE media. r=kaku
Duplicate of this bug: 1338045
https://hg.mozilla.org/mozilla-central/rev/1e059386f261
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Based on when this got filed, it certainly appears that 53 *should* be affected, but I'm not seeing any OrangeFactor data from mozilla-aurora to support it either. Can you please nominate this for Aurora approval assuming the underlying code defect is in fact there?
Flags: needinfo?(jwwang)
Approval Request Comment
[Feature/Bug causing the regression]:1331070
[User impact if declined]:mochitest timeout
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:The change is not complicated and has been tested on Try.
[String changes made/needed]:none
Flags: needinfo?(jwwang)
Attachment #8835838 - Flags: review+
Attachment #8835838 - Flags: approval-mozilla-aurora?
Comment on attachment 8835838 [details] [diff] [review]
1331070_fix_aurora.patch

Should help prevent test timeouts, let's uplift to aurora.
Attachment #8835838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1332019
Duplicate of this bug: 1331401
Duplicate of this bug: 1331201
Duplicate of this bug: 1329638
Whiteboard: [stockwell fixed]
You need to log in before you can comment on or make changes to this bug.