1219178 - Intermittent test_seek_out_of_range.html | Test timed out.

Status: RESOLVED FIXED

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

Description

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/logviewer.html#?job_id=16375948&repo=mozilla-inbound 

00:18:48 INFO - 46731 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_seek_out_of_range.html | Test timed out.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

INFO [finished gizmo.mp4-10] remaining= small-shot.mp3-2
small-shot.mp3 didn't finish playback. Not sure if this is related to recent changes to MP3TrackDemuxer in bug 1163667.

Comment 2

[Jean-Yves Avenard [:jya]]

I've also seen this one dom/media/test/test_bug686942.html going intermittent recently (bug 896723 is still closed).

and each time, it was an mp3 file causing the timeout:
like here.

https://treeherder.mozilla.org/logviewer.html#?job_id=13128402&repo=try

most on linux.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

test_bug686942.html also involves seeking.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1219178-Add-out-of-range-seeking-MP3Demuxer-test.patch

With this patch, I'm adding explicit out-of-range seeking unit tests and they pass fine.

This is likely a regression from bug 1163667, so I will look into reproducing it locally.

Comment 5

[Intermittent Failures Robot]

18 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 13
* fx-team: 3
* b2g-inbound: 2

Platform breakdown:
* osx-10-6: 18

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-10-28&endday=2015-10-28&tree=all

Comment 6

[Intermittent Failures Robot]

54 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 33
* fx-team: 7
* b2g-inbound: 5
* try: 3
* mozilla-central: 3
* mozilla-aurora: 3

Platform breakdown:
* osx-10-6: 54

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-10-26&endday=2015-11-01&tree=all

Comment 7

[Intermittent Failures Robot]

15 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 8
* fx-team: 4
* try: 1
* mozilla-aurora: 1
* b2g-inbound: 1

Platform breakdown:
* osx-10-6: 15

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-02&endday=2015-11-02&tree=all

Comment 8

[Intermittent Failures Robot]

68 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 37
* fx-team: 16
* mozilla-central: 5
* try: 4
* mozilla-aurora: 3
* b2g-inbound: 3

Platform breakdown:
* osx-10-6: 68

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-02&endday=2015-11-08&tree=all

Comment 9

[Eugen Sawin [:esawin]]

I think I understand what's going on here. The file used for the test (small-shot.mp3) is less then a second long, contains only 12 frames while having TOC headers with 100 entries (100 percent-based offset mappings).

When seeking based on the TOC entries we end up in the middle of the frame payloads, which happen to contain a lot of valid frame header patterns (false positives). We pass the bogus frames to the decoder which aborts decoding.

Although it's less likely to happen with regular files, we currently don't do anything to prevent it.
So, I'm looking into clamping the TOC-based seeking to where we would expect the frame headers instead of trusting the TOC-derived offsets.

We could also further restrict our frame header validation to reduce the chance for false positive detection, but that's not completely avoidable according to the MPEG standard.

I'll also look into making the decoder interaction more robust to that kind of failures in the refactoring bug 1221991.

Comment 10

[Intermittent Failures Robot]

19 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 11
* mozilla-central: 3
* fx-team: 3
* mozilla-aurora: 1
* b2g-inbound: 1

Platform breakdown:
* osx-10-6: 19

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-10&endday=2015-11-10&tree=all

Comment 11

[Intermittent Failures Robot]

16 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 9
* b2g-inbound: 4
* fx-team: 2
* mozilla-central: 1

Platform breakdown:
* osx-10-6: 16

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-12&endday=2015-11-12&tree=all

Comment 12

[Intermittent Failures Robot]

80 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 46
* fx-team: 14
* b2g-inbound: 7
* mozilla-central: 6
* mozilla-aurora: 5
* try: 2

Platform breakdown:
* osx-10-6: 80

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-09&endday=2015-11-15&tree=all

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1219178-1.1-Prevent-ScanUntil-from-overshooting-.patch

Exit early if scanning for the next frame would put us behind the target time frame.

This should help prevent overshooting.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1219178-2.1-Add-frame-consistency-verification-t.patch

We can only estimate the frame boundaries, so we need mechanics to recognize false-positive frame detection in frame payloads.

This adds a basic mechanism to verify consistency of frame meta data which should stay constant between frames of a stream.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1219178-3.1-Clamp-seeking-offsets-to-estimated-f.patch

Currently, FastSeek estimates the target offset based on average frame lengths or TOC and derives the frame index from that. This has the effect that even with perfect estimates, we would likely seek into frame payload offsets, instead of frame begins, which increases the likelihood of false-positive detection.

With this patch, we estimate the target frame index based on the same mechanics as before and then derive the target offset from that. This should put us closer to frame boundaries, depending on the quality of the estimates and is generally more consistent with the frame-based handling within the demuxer.

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1219178-4.1-Accept-only-layer-III-frames.-r-jya.patch

Let's further constrain frame validation by only accepting MP3 frames...

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1219178-5.1-Return-invalid-resource-length-when-.patch

This patch prevents the new tests (to follow) from crashing on initialization (RAII could be a better pattern to solve this, but it's just a test helper class).

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0006-Bug-1219178-6.1-Add-small-test-file-case.-r-jya.patch

This patch adds the small MP3 test file from the failing intermittent test as a unit test case.

This makes yet another copy from the file (there are already two) but it's only 8kB and it's safer not to depend on other tests' resources.

Comment 19

[Eugen Sawin [:esawin]]

Attachment: 0007-Bug-1219178-7.1-Add-out-of-bounds-seeking-test.-r-jy.patch

This is an out-of-bounds seeking test, which should help to prevent regressions on similar issues.

Comment 20

[Eugen Sawin [:esawin]]

Attachment: 0008-Bug-1219178-8.1-Constrain-seek-position-getter-to-ma.patch

We should never report a duration past the total duration, this is only used in tests.

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8688509 [details] [diff] [review]
0001-Bug-1219178-1.1-Prevent-ScanUntil-from-overshooting-.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +247,5 @@
>    }
>  
> +  if (Duration(mFrameIndex + 1) > aTime) {
> +    return SeekPosition();
> +  }

I'm not sure I follow the logic here.

So if duration is past where we want to seek to, you abort?

@@ +253,2 @@
>    MediaByteRange nextRange = FindNextFrame();
>    while (SkipNextFrame(nextRange) && Duration(mFrameIndex + 1) < aTime) {

what about swapping the two instead?

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8688511 [details] [diff] [review]
0002-Bug-1219178-2.1-Add-frame-consistency-verification-t.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +374,5 @@
>  }
>  
> +static bool
> +VerifyFrameConsistency(
> +    const FrameParser::Frame& aFrame1, const FrameParser::Frame& aFrame2) {

VerifyFrameConsistency(const FrameParser::Frame& aFrame1,
                       const FrameParser::Frame& aFrame2)

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8688518 [details] [diff] [review]
0003-Bug-1219178-3.1-Clamp-seeking-offsets-to-estimated-f.patch

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

::: dom/media/MP3Demuxer.cpp
@@ +530,4 @@
>  }
>  
>  int64_t
> +MP3TrackDemuxer::OffsetFromFrameIndex(int64_t aFrameIndex) const {

{ should be on the following line.. but I guess it's too late by now

Comment 24

[Eugen Sawin [:esawin]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> I'm not sure I follow the logic here.
> 
> So if duration is past where we want to seek to, you abort?

If the *following* frame is past the seek target, we don't scan forward because we would overshoot.
There is a hidden assumption that the current frame is timed earlier than the seek target, so the actual seek target may not correspond to an actual frame start (positioned between the current frame and the following frame).

> @@ +253,2 @@
> >    MediaByteRange nextRange = FindNextFrame();
> >    while (SkipNextFrame(nextRange) && Duration(mFrameIndex + 1) < aTime) {
> 
> what about swapping the two instead?

We shouldn't swap this because FindNextFrame() and {Skip|Get}NextFrame should always come in pairs. FindNextFrame moves the demuxing offset.

But you have a point, I should refactor ScanUntil or replace it with something more sensible since seeking mechanics have changed a bit (it's on my TODO list).

Comment 25

[Eugen Sawin [:esawin]]

(In reply to Jean-Yves Avenard [:jya] from comment #23)
> { should be on the following line.. but I guess it's too late by now

It's never too late! I'm keeping it consistent within the file for now and will patch the style in one go (prepare for a review :).

Comment 26

[Intermittent Failures Robot]

63 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 35
* fx-team: 11
* b2g-inbound: 7
* mozilla-aurora: 5
* mozilla-central: 3
* try: 2

Platform breakdown:
* osx-10-6: 63

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-16&endday=2015-11-22&tree=all

Comment 27

[Eugen Sawin [:esawin]]

Comment on attachment 8688509 [details] [diff] [review]
0001-Bug-1219178-1.1-Prevent-ScanUntil-from-overshooting-.patch

See comment 24 for description.

Comment 28

[Eugen Sawin [:esawin]]

Lots of failures, but nothing specific to media seeking: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e27b3a43e79

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/487f8bd6639b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2ec61e15cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c875b707350
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7416320e6ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/af7d739c7efc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b167a7dd195e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5d032e2bbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/662635b6ed18

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/487f8bd6639b
https://hg.mozilla.org/mozilla-central/rev/1b2ec61e15cc
https://hg.mozilla.org/mozilla-central/rev/3c875b707350
https://hg.mozilla.org/mozilla-central/rev/a7416320e6ed
https://hg.mozilla.org/mozilla-central/rev/af7d739c7efc
https://hg.mozilla.org/mozilla-central/rev/b167a7dd195e
https://hg.mozilla.org/mozilla-central/rev/fa5d032e2bbe
https://hg.mozilla.org/mozilla-central/rev/662635b6ed18

Comment 31

[Brad Jackson]

My build is broken with errors about classes with no SeekPosition or PrevFrame members. I compile with --disable-tests and it looks like these are #ifdef ENABLE_TESTS, but some code outside the ifdef calls them.

Comment 32

[Eugen Sawin [:esawin]]

Attachment: 0009-Bug-1219178-9.1-Make-SeekPosition-available-with-tes.patch

(In reply to Brad Jackson from comment #31)
> My build is broken with errors about classes with no SeekPosition or
> PrevFrame members. I compile with --disable-tests and it looks like these
> are #ifdef ENABLE_TESTS, but some code outside the ifdef calls them.

Sorry for the inconvenience, this patch will fix the issue. I will try to land it asap.
Thanks for reporting!

Comment 33

[Carsten Book [:Tomcat]]

(In reply to Eugen Sawin [:esawin] from comment #32)
> Created attachment 8691950 [details] [diff] [review]
> 0009-Bug-1219178-9.1-Make-SeekPosition-available-with-tes.patch
> 
> (In reply to Brad Jackson from comment #31)
> > My build is broken with errors about classes with no SeekPosition or
> > PrevFrame members. I compile with --disable-tests and it looks like these
> > are #ifdef ENABLE_TESTS, but some code outside the ifdef calls them.
> 
> Sorry for the inconvenience, this patch will fix the issue. I will try to
> land it asap.
> Thanks for reporting!

landed on m-c as https://hg.mozilla.org/mozilla-central/rev/35916735b8af

Comment 34

[Intermittent Failures Robot]

48 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 34
* fx-team: 7
* b2g-inbound: 4
* try: 1
* mozilla-central: 1
* mozilla-aurora: 1

Platform breakdown:
* osx-10-6: 47
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-23&endday=2015-11-29&tree=all

Comment 35

[Jean-Yves Avenard [:jya]]

I'm still seeing some on inbound...
https://treeherder.mozilla.org/logviewer.html#?job_id=18318133&repo=mozilla-inbound

Comment 36

[Intermittent Failures Robot]

47 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 27
* fx-team: 10
* mozilla-aurora: 7
* try: 1
* mozilla-central: 1
* b2g-inbound: 1

Platform breakdown:
* osx-10-6: 47

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-11-30&endday=2015-12-06&tree=all

Comment 37

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 6
* mozilla-aurora: 1

Platform breakdown:
* osx-10-6: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2015-12-07&endday=2015-12-13&tree=all

Comment 38

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-beta: 5

Platform breakdown:
* osx-10-6: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1219178&startday=2016-01-11&endday=2016-01-17&tree=all

Comment 39

[Eugen Sawin [:esawin]]

No time outs since 7th December. Closing.