Last Comment Bug 462959 - Implement nsIDOMHTMLMediaElement::GetPlayed()
: Implement nsIDOMHTMLMediaElement::GetPlayed()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Paul Adenot (:padenot)
:
Mentors:
: 700074 737030 (view as bug list)
Depends on: 635726
Blocks: AreWePlayingYet video 476984
  Show dependency treegraph
 
Reported: 2008-11-03 17:06 PST by Chris Pearce (:cpearce)
Modified: 2012-05-17 08:02 PDT (History)
20 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 for Bug 462959 (11.74 KB, patch)
2011-05-17 09:19 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v2 for Bug 462959 (11.91 KB, patch)
2011-05-19 11:20 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v3 (12.01 KB, patch)
2011-05-23 09:46 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Testing v2 for bug 462959 (5.65 KB, patch)
2011-05-23 09:46 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v4, adressing Ms2ger concerns (12.18 KB, patch)
2011-05-23 10:54 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v5, adressing :kinetik concern's (10.59 KB, patch)
2011-05-24 03:36 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Review
Patch v3 - test rewritten without setInterval or setTimeout (5.65 KB, patch)
2011-05-25 08:17 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v4 - with the good file. (6.45 KB, patch)
2011-05-26 01:53 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v5, using the mediatestmanager to provide files (7.80 KB, patch)
2011-05-30 04:20 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v6 (8.59 KB, patch)
2011-06-07 05:53 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v7 (8.81 KB, patch)
2011-06-08 03:42 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Patch v6 - Fixed a bug. (10.22 KB, patch)
2011-06-17 10:05 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v7 - Added a test about the bugfix (10.45 KB, patch)
2011-06-17 10:08 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v8 - Including Chris fix + nitpick (10.23 KB, patch)
2011-06-22 07:31 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v7 (11.40 KB, patch)
2011-06-27 05:54 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Review
Patch v9 (9.23 KB, patch)
2011-06-27 05:56 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Review
Patch v10 -- Typo fixed (9.23 KB, patch)
2011-06-28 11:14 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Review
Part 1 - core feature (11.48 KB, patch)
2011-08-02 04:43 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
part 2 - Tests (9.26 KB, patch)
2011-08-02 04:44 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Rebased feature patch. (10.72 KB, patch)
2012-04-23 11:29 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Tests (10.72 KB, patch)
2012-04-23 11:29 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Addressed cpearce comments. (12.94 KB, patch)
2012-04-26 12:12 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Tests (10.11 KB, patch)
2012-04-26 12:12 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Review
Addressed cpearce comments. (12.87 KB, patch)
2012-04-27 11:23 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Tests (8.89 KB, patch)
2012-04-27 11:23 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Review
Style change, as per Ms2ger remarks. (11.98 KB, patch)
2012-04-30 13:24 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review

Description Chris Pearce (:cpearce) 2008-11-03 17:06:22 PST

    
Comment 1 Chris Pearce (:cpearce) 2008-11-03 18:11:00 PST
You'll need a nsIDOMHTMLTimeRanges interface to implement this. Stubs for that are removed in patch for bug 462953.
Comment 2 Paul Adenot (:padenot) 2011-05-17 09:19:26 PDT
Created attachment 532989 [details] [diff] [review]
Patch v1 for Bug 462959

I'm going to add a test for this patch, but I need to learn how to use the test framework first.
Comment 3 Paul Adenot (:padenot) 2011-05-19 11:20:27 PDT
Created attachment 533718 [details] [diff] [review]
Patch v2 for Bug 462959
Comment 4 Paul Adenot (:padenot) 2011-05-19 11:21:49 PDT
Comment on attachment 533718 [details] [diff] [review]
Patch v2 for Bug 462959

Fixed a tiny bug I didn't catch.
Comment 5 Paul Adenot (:padenot) 2011-05-23 09:46:16 PDT
Created attachment 534456 [details] [diff] [review]
Patch v3
Comment 6 Paul Adenot (:padenot) 2011-05-23 09:46:57 PDT
Created attachment 534457 [details] [diff] [review]
Testing v2 for bug 462959
Comment 7 :Ms2ger 2011-05-23 09:52:17 PDT
A few comments:

Read the style guide: <https://developer.mozilla.org/En/Developer_Guide/Coding_Style>, in particular

if (..) {

}

instead of

if(..)
{

}

Same for for-loops, and in |for(PRUint32 i = 0 ;...|, drop the space before the semicolon.

Don't change iff, it's correct.

when the stream has ever been played -> if the stream has ever been played.

Lose the new newline in nsHTMLMediaElement::Play.

Use NS_MAX instead of PR_MAX.

Rev the uuid of nsIDOMHTMLMediaElement.
Comment 8 Paul Adenot (:padenot) 2011-05-23 10:54:21 PDT
Created attachment 534484 [details] [diff] [review]
Patch v4, adressing Ms2ger concerns
Comment 9 Matthew Gregan [:kinetik] 2011-05-23 15:49:31 PDT
This needs tests added to content/media/test.

mRangePlay might be better named mCurrentPlayRangeStart (or, if you can think of a shorter name that makes it clear it's the start of the current play range, use that instead).

+  // Update the mPlayed membed.

Typo - member?  I don't think this comment is necessary though.

I'm not sure I understand what mHasPlayed is for.  It seems like you use |mRangePlay == -1| instead.

+  if(mRangePlay == -1.0) // first time we play
+    GetCurrentTime(&mRangePlay);
+

Please use {} brackets here, and remove the comment.

Thanks for doing this!
Comment 10 Paul Adenot (:padenot) 2011-05-24 03:36:59 PDT
Created attachment 534709 [details] [diff] [review]
Patch v5, adressing :kinetik concern's

The mHasPlayed member was indeed unecessary, thanks for pointing that out.
Comment 11 Matthew Gregan [:kinetik] 2011-05-24 04:48:03 PDT
Comment on attachment 534709 [details] [diff] [review]
Patch v5, adressing :kinetik concern's

Thanks.  I just realized you have already attached a mochitest for this in a separate patch. I'll review that in the morning.
Comment 12 Matthew Gregan [:kinetik] 2011-05-24 22:20:41 PDT
I think the way you're using waitForExplicitFinish() and finish() in the test will result in the test finishing the first time finish() is called.  Unless I'm missing something, there's only a single SimpleTest for the entire page.  Take a look at content/media/test/test_load.html, which uses a counter of running tests to determine when to call finish().

You might need to use a longer file to get good test coverage.  I'm worried that a 1 second file will mean that, e.g. play_pause could easily play through to the end once before the timer runs due to high load (and thus delayed event execution) on the mochitest servers.  In fact, it'd be better to rewrite these tests to avoid setTimeout altogether, because it usually results in unreliable tests under load.  See https://developer.mozilla.org/en/QA/Avoiding_intermittent_oranges
Comment 13 Paul Adenot (:padenot) 2011-05-25 08:17:37 PDT
Created attachment 535067 [details] [diff] [review]
Patch v3 - test rewritten without setInterval or setTimeout

I've rewritten the tests, in order to remove all the calls to setInterval and setTimout. The tests should be machine-speed independent and much more reliable, now.

The duration of the file I use was 9.3 seconds, (not 1 sec), and that should be long enough to avoid the problems you mentioned.

I've also refactored the tests using the MediaTestManager class, provided in the manifest.js file, in the content/media/test directory, which implements what you proposed.
Comment 14 Matthew Gregan [:kinetik] 2011-05-25 15:31:15 PDT
I think you attached an old version of the patch.
Comment 15 Paul Adenot (:padenot) 2011-05-26 01:53:38 PDT
Created attachment 535281 [details] [diff] [review]
Patch v4 - with the good file.

Indeed. Here is the good file.
Comment 16 Matthew Gregan [:kinetik] 2011-05-26 16:49:33 PDT
I haven't completely reviewed it yet, but one thing I noticed:

+manager.runTests(gAudioTests, initial_value);

This causes the initial_value test to be called once for each file in gAudioTests, but then you're using AUDIO_FILE inside the test:

+  a1.src=AUDIO_FILE;

You can add a new g*Tests in manifest.js for this, or you can use one of the existing ones.  The advantage of using an existing one is that the tests will run with multiple decoder backends--which doesn't matter much for this code, but it could in the future.
Comment 17 Paul Adenot (:padenot) 2011-05-30 04:20:50 PDT
Created attachment 536047 [details] [diff] [review]
Patch v5, using the mediatestmanager to provide files
Comment 18 Chris Pearce (:cpearce) 2011-06-06 15:38:13 PDT
Comment on attachment 536047 [details] [diff] [review]
Patch v5, using the mediatestmanager to provide files

 
>+// Used by test_played. We need a file long enough to avoid random orange
>+var gPlayedTests = [
>+  { name:"big.wav", type:"audio/x-wav", duration:9.0, size:102444 },
>+  { name:"bogus.duh", type:"bogus/duh" }
>+];


You should include at least one Ogg and one WebM file as well, and the tests should run on video as well as audio to ensure that this feature works across the different decoder back ends.

You can see an example in test_bug495300.html at line 36 of how to create the appropriate audio/video element based on the test's "type" field.


>+// Whithout playing, check that player.played.length == 0
>+manager.runTests(gPlayedTests, initial_value);
>+// Play the file, test the range we have
>+manager.runTests(gPlayedTests, no_merge);
>+// Play the first half of the file, pause, play
>+// an check we have only one range
>+manager.runTests(gPlayedTests,  play_pause);
>+// Play the first half of the file, seek back, while
>+// continuing to play. We shall have only one range
>+manager.runTests(gPlayedTests, play_seek);
>+// Play and seek to have two ranges, and check that, as well a
>+// boudaries
>+manager.runTests(gPlayedTests, multiple_range_nomerge);
>+// Play and seek to have to overlapping ranges. They should be merged, to a
>+// range spanning all the test audio file.
>+manager.runTests(gPlayedTests, multiple_range_merge);
>+// Play to create two ranges, in the revers order. check that they are sorted.
>+manager.runTests(gPlayedTests, multiple_range_sort);

MediaTestManager.runTests() is not synchronous. It also resets its tokens array when called, so every time it's called it will wipe out history of previously started tests. I'm guessing that's why you need long media files to ensure you don't get random failures. If you want to run multiple tests in a single MediaTestManager, you need to take an approach similar to how test_preload_actions.html does it, i.e. create an array of closures, and call MediaTestManager.runTests() once only.
Comment 19 Paul Adenot (:padenot) 2011-06-07 05:53:32 PDT
Created attachment 537774 [details] [diff] [review]
Patch v6

Ok, I guess finally understood the point of MediaTestManager.

The tests are now running using a single |runTests()| call, and are ran 4 times (wav audio, ogg audio, ogg video, webm video) each, to test all backends.
Comment 20 Chris Pearce (:cpearce) 2011-06-07 15:02:22 PDT
Comment on attachment 537774 [details] [diff] [review]
Patch v6

Getting there. ;)

The purpose of MediaTestManager is ultimately to make parallelism tunable in our media tests. This is so we can change the value of PARALLEL_TESTS in manifest.js to control how many tests are run at once. Each <audio> or <video> element (currently) requires 4 threads to run, and on Linux x86 each thread has 10MB virtual address space reserved for each thread stack. When there are a lot of media elements around, we can acutally run out of address space, and this has caused intermittent test failures on Linux x86. This is why many tests reset the elements src to "" and remove the element from the document; we're trying to destory the decoders and force them to be GC'd to release their memory.

(To this end, can you set src="" in your finish_test() function? Note this may fire some events, so it may cause test failures if you've got listeners for those events, in which case don't bother.)

To allow the parallelism to be tuned, only *one* test should be started each time the MediaTestManager calls your startTest() function. At the moment you're running four tests per startTest() call.

You can achieve this by creating a test array with multiple entries "per test", which vary in what resource they're loading. e.g. something like:

function createTestArray(tests) {
  var A = [];
  for (var i=0; i<tests.length; i++) {
    for (var k=0; k<gPlayedTests.length; k++) {
      var t = new Object();
      t.setup = tests[i].setup;
      t.name = gPlayedTests[k].name;
      t.type = gPlayedTests[k].type;
      A.push(t);
    }
  }
  return A;
}

function startTest(test, token) {
  var elemType = /^audio/.test(test.type) ? "audio" : "video";
  var element = document.createElement(elemType);
  element.src = test.name;
  element.token = token;
  test.setup(element);
  manager.started(token);
}

manager.runTests(createTestArray(tests, startTest));


Note that every time startTest() is called, it's passed a unique token, so you should call manager.started(token) exactly once and manager.finished(token) exactly once for each token (and thus test) that's run. ATM you're reusing each token passed to startTest() 4 times.

Do you need to have a "size" field on some of the members of gPlayedTests?


+// Play and seek to have to overlapping ranges. They should be merged, to a
+// range spanning all the test audio file.

In this test, and others, it's probably best to start playback in the "loadedmetadata" handler, just in case we manage to play through before the "loadedmetadata" or "timeupdate" handler have a chance to run (it could for really small files, or if the main thread is under heavy load).


+// Play to create two ranges, in the revers order. check that they are sorted.

"reverse order".

Other than that, the tests themselves look good.
Comment 21 Paul Adenot (:padenot) 2011-06-08 03:42:51 PDT
Created attachment 537976 [details] [diff] [review]
Patch v7

Patch v7, I've checked that the tokens are not reused, and fixed the other problem you mentioned.
Comment 22 Chris Pearce (:cpearce) 2011-06-09 20:47:12 PDT
Comment on attachment 537976 [details] [diff] [review]
Patch v7


>diff --git a/content/media/test/test_played.html b/content/media/test/test_played.html
>new file mode 100644
>--- /dev/null
>+++ b/content/media/test/test_played.html
>@@ -0,0 +1,209 @@
>+
>+var test = getPlayableVideo(gPlayedTests);

test is unused?

Otherwise looks good!
Comment 23 :Ms2ger 2011-06-10 10:30:22 PDT
I pushed this to try, and the test passed on Linux, but failed on most OSX and Windows builds. (Mainly rounding errors, apparently. See <http://tbpl.mozilla.org/?tree=Try&rev=f6a0c427accd>.
Comment 24 Paul Adenot (:padenot) 2011-06-17 10:05:52 PDT
Created attachment 540076 [details] [diff] [review]
Patch v6 - Fixed a bug.

I have fixed the rounding error issues, but a test keep failing on the try servers, only on OSX. I've checked today with a macbookpro, and it works quite nice, I've ran the test like 20 times, and it is always green. My only guess is that the system running the tests is under high cpu pressure, and the "timeupdate" events are delayed, and the audio stream ends. Again, I can't explain yet why it is happening only on macs, and only on the tryservers.

In the process, I've fixed a bug which created zero-width ranges when seeking without playing, and added a test case for this update.
Comment 25 Paul Adenot (:padenot) 2011-06-17 10:08:19 PDT
Created attachment 540079 [details] [diff] [review]
Patch v7 - Added a test about the bugfix
Comment 26 Chris Pearce (:cpearce) 2011-06-17 19:45:39 PDT
(In reply to comment #24)
> Created attachment 540076 [details] [diff] [review] [review]
> Patch v6 - Fixed a bug.
> 
> I have fixed the rounding error issues, but a test keep failing on the try
> servers, only on OSX.

Not much point in r+ing and landing this if it's got a test failure. :(

You can try setting DEBUG_TEST_LOOP_FOREVER = true in content/media/test/manifest.js, and running the test locally. Then since your test uses the MediaTestManager, it will loop forever until it fails. That can help when trying to reproduce/debug random failures. Running the test looping in multiple tabs will increase the CPU load and should reduce the amount of time required to repro the failure. Increasing PARALLEL_TESTS in manifest.js may reduce time to repro too.

Of course all that won't help if the failure actually is platform specific, then you've only got the tryserver and to help you out. :( Maybe try pushing a patch with debug logging to tryserver? stdout is captured in the tinderbox log.
Comment 27 Chris Pearce (:cpearce) 2011-06-20 21:02:19 PDT
(In reply to comment #24)
> Created attachment 540076 [details] [diff] [review] [review]
> Patch v6 - Fixed a bug.
> 
> I have fixed the rounding error issues, but a test keep failing on the try
> servers, only on OSX. 

I can reproduce some failures on my old mac mini at the office, I'll see if I can figure out what's causing them in the next day or so.
Comment 28 Chris Pearce (:cpearce) 2011-06-21 21:34:12 PDT
Comment on attachment 540079 [details] [diff] [review]
Patch v7 - Added a test about the bugfix


>+// Play to create two ranges, in the reverse order. check that they are sorted.
>+// The ranges created sould be as follow
>+{
>+  setup : function (element) {
>+    function end() {
>+      element.pause();
>+      let p = element.played;
>+      is(p.length, 2, "There should be two ranges");
>+      is(p.start(0), element.duration/6, "Start of first range should be the sixth of the duration");
>+      ok(p.end(0) > 3*element.duration/6, "End of first range should be greater than three times the sixth of the duration");
>+      is(p.start(1), 4 * element.duration/6, "Start of second range should be four times the sixth of the duration");
>+      ok(p.end(1) > 5*element.duration/6, "End of second range should be greater that five times the sixth of the duration");
>+      finish_test(element);
>+    }


The problem is here. By the time the timeupdate fires and you end the test, it's possible for playback to have played enough so that the two ranges have merged. So you'll have only 1 range, so the |p.start(1)| and |p.end(1)| calls will fail and throw an index out of bounds exception.

You can't rely on timeupdate events to be processed in a timely fashion. The best you can do is check that p.length>=1, and check that the start of the first range is duration/6 and the end of the last range (p.end(p.length-1)) is >= 5*duration/6. You can't do the other checks reliably.

I pushed that to TryServer, and it passed:
http://tbpl.mozilla.org/?tree=Try&rev=5b9b486ceb40

See http://hg.mozilla.org/try/rev/5b9b486ceb40 for the exact changes I added, you'll need to incorporate those changes or something equivalent into your patch.
Comment 29 Chris Pearce (:cpearce) 2011-06-21 21:40:02 PDT
Comment on attachment 540076 [details] [diff] [review]
Patch v6 - Fixed a bug.

Matthew reviewed the original patch, so he should look at this updated one.
Comment 30 Paul Adenot (:padenot) 2011-06-22 07:31:37 PDT
Created attachment 541040 [details] [diff] [review]
Patch v8 - Including Chris fix + nitpick

Here is the test patch updated. I've also fixed the caption which was not valid anymore.

Anyway, Chris, thanks for helping me fixing that.
Comment 31 Matthew Gregan [:kinetik] 2011-06-26 21:39:41 PDT
Can you please fix the new |if(foo)| instances?  The coding style dictates |if (foo)|, so please stick to that.

Also, merge the code fix included in the "test" patch into the patch containing the code changes please.

For the first test using timeupdate (that plays half the file), a more reliable way to test this may be to seek to duration/2 first, play the last half of the file, then (when ended is fired) seek to the beginning and play the first half, then check that played reports a single range.

In general, I'd prefer to have fewer tests that always test the expected behaviour, rather than many unreliable/timing dependent tests that only test the expected behaviour some of the time.

The test code:

+		element.addEventListener('loadedmetadata', function() {
+			element.currentTime =     element.duration / 5;
+			element.currentTime = 2 * element.duration / 5;
+			element.currentTime = 3 * element.duration / 5;
+			element.currentTime = 4 * element.duration / 5;
+			element.currentTime = 5 * element.duration / 5;
+		}, false);

doesn't really work, because it's not waiting for one seek to complete before starting another.  You need to start the next seek when a seeked event is fired.

Also, please fix the typos: Whithout, boudaries, sould, repeteadly,
Comment 32 Paul Adenot (:padenot) 2011-06-27 05:54:47 PDT
Created attachment 542136 [details] [diff] [review]
Patch v7
Comment 33 Paul Adenot (:padenot) 2011-06-27 05:56:21 PDT
Created attachment 542137 [details] [diff] [review]
Patch v9
Comment 34 Matthew Gregan [:kinetik] 2011-06-27 17:14:14 PDT
Comment on attachment 542137 [details] [diff] [review]
Patch v9

+// Seek repeteadly without playing. No range should appear.

One last typo.
Comment 35 Paul Adenot (:padenot) 2011-06-28 11:14:29 PDT
Created attachment 542525 [details] [diff] [review]
Patch v10 -- Typo fixed
Comment 36 :Ms2ger 2011-07-01 03:47:28 PDT
Comment on attachment 542525 [details] [diff] [review]
Patch v10 -- Typo fixed

>+    element.curretTime = element.duration / 2;

I fixed this typo and sent to try. I'll push it when I get the results.
Comment 37 :Ms2ger 2011-07-02 10:57:04 PDT
Unfortunately, that fixed assignment threw. I've now removed the line entirely and will push to try again.
Comment 39 Phil Ringnalda (:philor) 2011-07-03 18:31:29 PDT
Sorry, had to back it out in http://hg.mozilla.org/mozilla-central/rev/dd9a2ec82f68 for causing the same infinite assertions as in bug 635726 (which is why I mistakenly starred the first four failures as being that, before I finally realized it had to be something new).
Comment 40 Chris Pearce (:cpearce) 2011-07-03 22:10:00 PDT
While fixing bug 635726 I found another intermittent test failure in test_played:

failed | element.played.length == 2 - got 1, expected 2
failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount" code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)" location: "http://mochi.test:8888/tests/content/media/test/test_played.html?autorun=1&timeout=100000&logFile=c%3A%5CUsers%5Ccpearce%5Csrc%5Cmauve%5Cobjdir%5C_tests%5Ctesting%5Cmochitest.log&fileLevel=DEBUG&consoleLevel=DEBUG Line: 111"] at :0

This is in the "ended" handler for the "Play and seek to have two ranges, and check that, as well a boundaries" test.

The line numbers are slightly different because I added some instrumentation.

I reproduced this by setting DEBUG_TEST_LOOP_FOREVER=true in manifest.js, and running the test looping in 20 tabs.

You'll want the fix for bug 635726 applied before you try fixing this, I'll post that fix later on tonight or tomorrow.
Comment 41 Paul Adenot (:padenot) 2011-07-05 08:19:06 PDT
I can reproduce this, and I guess that we cannot test this reliably, it seems to be the same problem as before, we cannot be sure that |timeUpdate| will be fired in a timely fashion.

Maybe we could detect when the event has not been fired on time, and do something like this :

> element.addEventListener("ended", (function() {
>   if(element.played.length > 1) {
>     is(element.played.length, 2, "element.played.length == 2");
>     var guess = element.played.end(0) + element.duration/100.0;
>     ok(rangeCheck(element.played.start(1), guess), "we should have seeked forward by one tenth of the duration");
>     is(element.played.end(1), element.duration, "end of second range shall be the total duration");
>   }
>   is(element.played.start(0), 0, "start of first range shall be 0");
>   finish_test(element);
> }), false);

This way, we can test the behavior of the feature, but we are protected from random oranges.
Comment 42 Chris Pearce (:cpearce) 2011-07-05 15:17:22 PDT
That's probably good enough.
Comment 43 Paul Adenot (:padenot) 2011-07-06 06:31:52 PDT
I pushed comment 41 modifications to try (http://tbpl.mozilla.org/?tree=Try&rev=e8e3ba577ba7).
Comment 44 :Ms2ger 2011-08-01 11:11:01 PDT
Paul, AFAICT, there's no difference between attachment 542525 [details] [diff] [review] and <http://hg.mozilla.org/try/rev/f475d7fdf832>. I also don't see the code from comment 41 anywhere in the latter.
Comment 45 Paul Adenot (:padenot) 2011-08-02 04:43:32 PDT
Created attachment 550044 [details] [diff] [review]
Part 1 - core feature
Comment 46 Paul Adenot (:padenot) 2011-08-02 04:44:01 PDT
Created attachment 550045 [details] [diff] [review]
part 2 - Tests
Comment 49 Paul Adenot (:padenot) 2011-11-05 17:31:40 PDT
*** Bug 700074 has been marked as a duplicate of this bug. ***
Comment 50 Matthew Gregan [:kinetik] 2012-03-19 12:32:27 PDT
*** Bug 737030 has been marked as a duplicate of this bug. ***
Comment 51 Paul Adenot (:padenot) 2012-04-23 11:29:11 PDT
Created attachment 617555 [details] [diff] [review]
Rebased feature patch.

I've rebased this patch, it does not seem to fail anymore. As it was reviewed quite a long time ago, I've asked for review again.

Green try (apart from usual random orange) : https://tbpl.mozilla.org/?tree=Try&rev=d33ae585467c, with quite a few re-run to ensure correctness.
Comment 52 Paul Adenot (:padenot) 2012-04-23 11:29:56 PDT
Created attachment 617556 [details] [diff] [review]
Tests
Comment 53 Chris Pearce (:cpearce) 2012-04-25 21:51:16 PDT
Comment on attachment 617555 [details] [diff] [review]
Rebased feature patch.

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

No major problems here, just some minor style fixes. I'd like to see it again before r+ing it though.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +733,5 @@
>  
> +  // Range of time played.
> +  nsTimeRanges mPlayed;
> +
> +  // Temporary variable for storing a time, when the stream starts to play

Full stop at end of sentences please.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1132,5 @@
>  NS_IMETHODIMP nsHTMLMediaElement::SetCurrentTime(double aCurrentTime)
>  {
>    StopSuspendingAfterFirstFrame();
>  
> +  if (mCurrentPlayRangeStart != -1) {

May as well use -1.0 and 0.0 to be consistent here?

@@ +1133,5 @@
>  {
>    StopSuspendingAfterFirstFrame();
>  
> +  if (mCurrentPlayRangeStart != -1) {
> +    double oldCurrentTime = 0;

This is still the current time throughout its usage, so calling it "old" isn't quite right. I'd call it "rangeEndTime" or somesuch, to denote its purpose.

@@ +1135,5 @@
>  
> +  if (mCurrentPlayRangeStart != -1) {
> +    double oldCurrentTime = 0;
> +    GetCurrentTime(&oldCurrentTime);
> +    LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime));

Follow the convention of the other LOG calls, i.e. LOG(PR_LOG_DEBUG, "%p ....", this, ...). This enables us to distinguish what log message belongs to what element when we have multiple media elements running at the same time.

I'd also say "Adding a 'played' range: "..., so it's clear what kind of range you're talking about here.

@@ +1136,5 @@
> +  if (mCurrentPlayRangeStart != -1) {
> +    double oldCurrentTime = 0;
> +    GetCurrentTime(&oldCurrentTime);
> +    LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime));
> +    // Multiple seek without playing

This also happens when you play and then seek, not only after multiple seeks without playing...

Can you also end sentences with a full stop please? Thanks.

@@ +1137,5 @@
> +    double oldCurrentTime = 0;
> +    GetCurrentTime(&oldCurrentTime);
> +    LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime));
> +    // Multiple seek without playing
> +    if (mCurrentPlayRangeStart != oldCurrentTime)

{} around if statement blocks please, unless it's a trivial error return.

@@ +1170,5 @@
>    // event if it changes the playback position as a result of the seek.
>    LOG(PR_LOG_DEBUG, ("%p SetCurrentTime(%f) starting seek", this, aCurrentTime));
>    nsresult rv = mDecoder->Seek(clampedTime);
>  
> +  // Start a new range at position we seeked to

Full stop at end of sentences please.

@@ +1226,5 @@
> +  if (mCurrentPlayRangeStart != -1.0) {
> +    double now = 0.0;
> +    GetCurrentTime(&now);
> +    if (mCurrentPlayRangeStart != now)
> +      ranges->Add(mCurrentPlayRangeStart, now);

{} around if statement bodies please.

@@ +2827,5 @@
>      QueueSelectResourceTask();
>    }
>  
>    // A load was paused in the resource selection algorithm, waiting for
> +  // a new source child to be added, resume the resource selection algorithm.

Good spotting.

::: content/html/content/src/nsTimeRanges.cpp
@@ +82,5 @@
>  }
>  
>  void
>  nsTimeRanges::Add(double aStart, double aEnd) {
>    mRanges.AppendElement(TimeRange(aStart,aEnd));

Can you change this so that if aStart > aEnd, we don't add the range? We should warn with NS_WARNING() when someone tries to do this. Thanks.

@@ +96,5 @@
> +    // This merges the intervals
> +    TimeRange current(mRanges[0]);
> +    for (PRUint32 i = 1; i < mRanges.Length(); i++) {
> +      if (current.mEnd >= mRanges[i].mStart) {
> +        current.mEnd = NS_MAX(current.mEnd, mRanges[i].mEnd);

current.mEnd = mRanges[i].mEnd;
(since we know that a range's end must be after its start thanks to the change to nsTimeRanges::Add() above.)

::: content/html/content/src/nsTimeRanges.h
@@ +56,5 @@
>    void Add(double aStart, double aEnd);
>  
> +  // See
> +  // http://www.whatwg.org/specs/web-apps/current-work/multipage/
> +  //                              the-iframe-element.html#normalized-timeranges-object

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#normalized-timeranges-object

@@ +71,5 @@
>    };
>  
> +  struct CompareTimeRanges
> +  {
> +    PRBool Equals(const TimeRange& tr1, const TimeRange& tr2) const

aTr1, aTr2.

Sorry, aOurSillyNamingConvention applies here too.

@@ +83,5 @@
> +    {
> +      return tr1.mStart < tr2.mStart;
> +    }
> +  };
> +

Too many extra line breaks.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +56,5 @@
>  #undef GetCurrentTime
>  #endif
>  %}
>  
> +[scriptable, uuid(11d560c7-4fc6-41e1-bb0e-de0255f1e8b1)]

You also need to rev the uuids for elements which inherit from this one, so audio and video elements.

There's a script to do this for you BTW:
http://people.mozilla.org/~sfink/uploads/update-uuids

@@ +90,5 @@
>             attribute double currentTime;
>    readonly attribute double initialTime;
>    readonly attribute double duration;
>    readonly attribute boolean paused;
> +  readonly attribute nsIDOMTimeRanges played;

Leave the linebreak in.
Comment 54 :Ms2ger 2012-04-26 02:41:45 PDT
(In reply to Chris Pearce (:cpearce) from comment #53)
> ::: content/html/content/src/nsTimeRanges.h
> @@ +56,5 @@
> >    void Add(double aStart, double aEnd);
> >  
> > +  // See
> > +  // http://www.whatwg.org/specs/web-apps/current-work/multipage/
> > +  //                              the-iframe-element.html#normalized-timeranges-object
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-
> element.html#normalized-timeranges-object

A more reliable link (that doesn't rely on where the multipage spec is split) would be <http://www.whatwg.org/html/#normalized-timeranges-object>.
Comment 55 Paul Adenot (:padenot) 2012-04-26 12:12:15 PDT
Created attachment 618758 [details] [diff] [review]
Addressed cpearce comments.
Comment 56 Paul Adenot (:padenot) 2012-04-26 12:12:52 PDT
Created attachment 618759 [details] [diff] [review]
Tests
Comment 57 Chris Pearce (:cpearce) 2012-04-26 19:23:27 PDT
Comment on attachment 618758 [details] [diff] [review]
Addressed cpearce comments.

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

Better, but I picked up a few nits again. Sorry, I should have caught them last time.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +733,5 @@
>  
> +  // Range of time played.
> +  nsTimeRanges mPlayed;
> +
> +  // Temporary variable for storing a time, when the stream starts to play.

This isn't a "Temporary variable", in the classic sense if it was "temporary", it'd be a local variable right.

How about:

"Stores the time at the start of the current 'played' range."

::: content/html/content/src/nsTimeRanges.cpp
@@ +83,5 @@
>  
>  void
>  nsTimeRanges::Add(double aStart, double aEnd) {
> +  if (aStart < aEnd) {
> +    NS_WARNING("Can't add a range if the end is older that the start.");

Return here, so we *don't* add the range if its end is after its start.

::: content/html/content/src/nsTimeRanges.h
@@ +67,5 @@
>      double mStart;
>      double mEnd;
>    };
>  
> +  struct CompareTimeRanges

Local brace convention seems to be:

struct structName {
  type functionName() {
  }
};


Rather than braces on their own lines. The style varies between files/modules, follow the local convention in each file when adding code. Also we use bool rather than PRBool now.

@@ +74,5 @@
> +    {
> +      return aTr1.mStart == aTr2.mStart && aTr1.mEnd == aTr2.mEnd;
> +    }
> +
> +    // Here, we aim at time range normalization. That why we order only by start

"Here, we aim at time range normalization."; this doesn't quite follow. Better to just add a comment at the start of the class describing what the class does, and who uses it, e.g.:

"Comparator which orders TimeRanges by start time. Used by Normalize()."
Comment 58 Chris Pearce (:cpearce) 2012-04-26 20:08:13 PDT
Comment on attachment 618759 [details] [diff] [review]
Tests

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

It seems one of the tests has a spelling mistake which is the only thing preventing it from failing. Yay dynamic languages.

::: content/html/content/src/nsTimeRanges.cpp
@@ +82,5 @@
>  }
>  
>  void
>  nsTimeRanges::Add(double aStart, double aEnd) {
> +  if (aStart > aEnd) {

This should be corrected in the previous patch, along with returning here instead of adding the incorrect range.

::: content/media/test/manifest.js
@@ +27,5 @@
> +var gPlayedTests = [
> +  { name:"big.wav", type:"audio/x-wav", duration:9.0, size:102444 },
> +  { name:"sound.ogg", type:"audio/ogg", duration:4.0, size:2603},
> +  { name:"seek.ogv", type:"video/ogg", duration:3.966, size:285310},
> +  { name:"seek.webm", type:"video/webm", duration:3.966 },

Why does this not have a size when the others do? Probably none of them need sizes?

::: content/media/test/test_played.html
@@ +52,5 @@
> +// Play the first half of the file, pause, play
> +// an check we have only one range.
> +{
> +  setup : function (element) {
> +    let onEnded = function() {

Aren't you playing the second half of the file through, then trying to replay the file again? i.e. the comment describing this test doesn't match reality? Oh wait, you misspelt element.curretTime below, so you're not seeking to duration/2.

You'd better fix this test.

Also, assuming you fix the spelling mistake, won't check_full_file_played's ended handler will run as well as onEnded on the first play through? Therefore, won't check_full_file_played's ended handler will finish the test, after only playing half the media, and nothing onEnded does will have any affect on the test? But check_full_file_played's ended handler checks that the entire media has played, but only half of it will have playes... How can this test possibly pass?

@@ +113,5 @@
> +    element.play();
> +  }
> +},
> +
> +// Play and seek to have to overlapping ranges. They should be merged, to a

Isn't this test the same as your " Play the first half of the file, seek back, while // continuing to play. We shall have only one range." test, except that your dividing element.duration  by 3 instead of 4??

@@ +226,5 @@
> +  test.setup(element);
> +  manager.started(token);
> +}
> +
> +manager.runTests(createTestArray(tests), startTest);

You don't need to pass tests here right? It's already declared in this scope?
Comment 59 Paul Adenot (:padenot) 2012-04-27 11:22:38 PDT
> ::: content/media/test/test_played.html
> @@ +52,5 @@
> > +// Play the first half of the file, pause, play
> > +// an check we have only one range.
> > +{
> > +  setup : function (element) {
> > +    let onEnded = function() {
> 
> Aren't you playing the second half of the file through, then trying to
> replay the file again? i.e. the comment describing this test doesn't match
> reality? Oh wait, you misspelt element.curretTime below, so you're not
> seeking to duration/2.
> 
> You'd better fix this test.
> 
> Also, assuming you fix the spelling mistake, won't check_full_file_played's
> ended handler will run as well as onEnded on the first play through?
> Therefore, won't check_full_file_played's ended handler will finish the
> test, after only playing half the media, and nothing onEnded does will have
> any affect on the test? But check_full_file_played's ended handler checks
> that the entire media has played, but only half of it will have playes...
> How can this test possibly pass?

The comment was indeed incorrect. The behavior that is tested in this particular test is :
- Go to the half of the file upon loading ;
- Play until the end ;
- Go to the beginning ;
- Play until the end ;
- Check that we have only one range.

At the moment, it was playing the file in its entirety twice, hence the test pass. If I fix only the typo, I get a failure for each media, which is logical.

> @@ +113,5 @@
> > +    element.play();
> > +  }
> > +},
> > +
> > +// Play and seek to have to overlapping ranges. They should be merged, to a
> 
> Isn't this test the same as your " Play the first half of the file, seek
> back, while // continuing to play. We shall have only one range." test,
> except that your dividing element.duration  by 3 instead of 4??

Absolutely, I've removed it this test.
Comment 60 Paul Adenot (:padenot) 2012-04-27 11:23:22 PDT
Created attachment 619115 [details] [diff] [review]
Addressed cpearce comments.
Comment 61 Paul Adenot (:padenot) 2012-04-27 11:23:56 PDT
Created attachment 619117 [details] [diff] [review]
Tests
Comment 62 Chris Pearce (:cpearce) 2012-04-29 18:58:36 PDT
Comment on attachment 619115 [details] [diff] [review]
Addressed cpearce comments.

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

Thanks, looks good!
Comment 63 Chris Pearce (:cpearce) 2012-04-29 19:02:55 PDT
Comment on attachment 619117 [details] [diff] [review]
Tests

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

Nice. Thanks!
Comment 64 :Ms2ger 2012-04-30 01:43:17 PDT
Comment on attachment 619115 [details] [diff] [review]
Addressed cpearce comments.

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

::: content/html/content/public/nsHTMLMediaElement.h
@@ +731,5 @@
>    // True if the sound is muted
>    bool mMuted;
>  
> +  // Range of time played.
> +  nsTimeRanges mPlayed;

This sucks for packing. Please put the two new members above the booleans.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1208,5 @@
>    return NS_OK;
>  }
>  
> +/* readonly attribute nsIDOMHTMLTimeRanges played; */
> +NS_IMETHODIMP nsHTMLMediaElement::GetPlayed(nsIDOMTimeRanges * *aPlayed)

nsIDOMTimeRanges** aPlayed

::: content/html/content/src/nsTimeRanges.cpp
@@ +90,5 @@
>    mRanges.AppendElement(TimeRange(aStart,aEnd));
>  }
> +
> +void
> +nsTimeRanges::Normalize() {

{ on the next line (yes, I know this is wrong in surrounding code too).
Comment 65 Paul Adenot (:padenot) 2012-04-30 13:24:28 PDT
Created attachment 619664 [details] [diff] [review]
Style change, as per Ms2ger remarks.

I suppose there is no need for a review this time.
Comment 67 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 21:21:44 PDT
https://hg.mozilla.org/mozilla-central/rev/5f9ab94a0e23

Note You need to log in before you can comment on or make changes to this bug.