Convert all MediaSource mochitests to use mp4 content

RESOLVED FIXED in Firefox 42

Status

()

P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
We will temporarily disable webm with mediasource while webm is being converted to the MP4Reader's API.

As such, all mochitests using webm should be converted to use mp4 content instead.

Updated

4 years ago
Priority: -- → P3
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 1

4 years ago
This only replicates the webm tests using mp4 content.
Attachment #8628029 - Flags: review?(karlt)
Comment on attachment 8628029 [details] [diff] [review]
Add MP4 mediasource mochitests.

Please use hg copy or git --find-copies-harder and/or -C options as necessary to identify copies in the patch.

>-      target.removeEventListener(name, cb);
>+      target.removeEventListener(name, arguments.callee);

Why is this change in this patch?
Attachment #8628029 - Flags: review?(karlt) → review-
(Assignee)

Comment 3

4 years ago
I tried that, and it can't be done as I'm using git. Consulted with glandiun and it can't be done with git.
(Assignee)

Comment 4

4 years ago
(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #2)
> Comment on attachment 8628029 [details] [diff] [review]
> Add MP4 mediasource mochitests.
> 
> >-      target.removeEventListener(name, cb);
> >+      target.removeEventListener(name, arguments.callee);
> 
> Why is this change in this patch?

Because the code is wrong and affected the results (I had to make use of once a few times in the new test)
As the event handler isn't removed from the element, the handler is called multiple times.
(Assignee)

Comment 5

4 years ago
Posted patch Clone files (obsolete) — Splinter Review
(Assignee)

Comment 6

4 years ago
This only replicates the webm tests using mp4 content.
Attachment #8631459 - Flags: review?(karlt)
(Assignee)

Comment 7

4 years ago
I split the patch in two commits to ease the review.

I will fold the two upon commit, the copy history will be lost, unless someone using hg wants to do it instead.
This is a patch to represent the effects of

> for file in test_*.html~test_*_mp4.html; [ -e $file:r_mp4.html ] || hg cp $file $file:r_mp4.html

Use this instead of adding new files, and we can expect merges and rebases to
do the right thing.  Don't fold into another patch with git unless you are
prepared to do what is necessary to fix up the patch.

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> I tried that, and it can't be done as I'm using git. Consulted with glandiun
> and it can't be done with git.

I don't believe it can't be done.  If git is too slow or too memory hungry
with -l0 --find-copies --find-copies-harder on a project the size of mozilla,
then some persuasion may be required, but it is a text file with a simple
format so there will be a way to do it.

However, keeping the clone and changes separate provides most of the benefits,
so that is good enough here.
Attachment #8628029 - Attachment is obsolete: true
Attachment #8631458 - Attachment is obsolete: true
Comment on attachment 8631459 [details] [diff] [review]
Add MP4 mediasource mochitests.

>Bug 1148103: Add MP4 mediasource mochitests. r=karlt
>
>This only replicates the webm tests using mp4 content.

There are some changes here that are not consistent with the checkin comment.

> function once(target, name, cb) {
>   var p = new Promise(function(resolve, reject) {
>     target.addEventListener(name, function() {
>-      target.removeEventListener(name, cb);
>+      target.removeEventListener(name, arguments.callee);
>       resolve();
>     });

This looks good thanks, but please move to a separate patch as the bug is not
directly mp4 specific.

>--- a/dom/media/mediasource/test/test_HaveMetadataUnbufferedSeek_mp4.html
>+++ b/dom/media/mediasource/test/test_HaveMetadataUnbufferedSeek_mp4.html

>-    v.addEventListener("loadeddata", function onloadeddata() {
>-      v.removeEventListener("loadeddata", onloadeddata);
>-      ok(v.readyState >= v.HAVE_CURRENT_DATA, "readyState is >= CURRENT_DATA");
>+    v.addEventListener("canplay", function oncanplay() {
>+      v.removeEventListener("canplay", oncanplay);
>+      ok(v.readyState >= v.HAVE_FUTURE_DATA, "readyState is >= FUTURE_DATA");
>       v.currentTime = target;

I suspect this might be accidentally reverting 4c68f224513a.
Attachment #8631459 - Flags: review?(karlt) → review-
(Assignee)

Comment 10

4 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #8)
> Created attachment 8631972 [details] [diff] [review]
> clone webm-only mediasource tests for mp4 tests
> 
> This is a patch to represent the effects of
> 
> > for file in test_*.html~test_*_mp4.html; [ -e $file:r_mp4.html ] || hg cp $file $file:r_mp4.html
> 
> Use this instead of adding new files, and we can expect merges and rebases to
> do the right thing.  Don't fold into another patch with git unless you are
> prepared to do what is necessary to fix up the patch.

As I mentioned, I now use git.
Git doesn't support copy nor rename.

> 
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > I tried that, and it can't be done as I'm using git. Consulted with glandiun
> > and it can't be done with git.
> 
> I don't believe it can't be done.  If git is too slow or too memory hungry
> with -l0 --find-copies --find-copies-harder on a project the size of mozilla,
> then some persuasion may be required, but it is a text file with a simple
> format so there will be a way to do it.
> 

As soon as I add a binary file (here bipbop2s.mp4) to the commit, --find-copies stop working.

As I need this file for the test to work, splitting this or the addition of bipbop2s.mp4 are kind of the same really.

I've spent a good hour attempting to find ways to get around it with no luck.
And regardless of the patch produced, as soon as I would push to the tree, those changes would be lost as git-cinnabar doesn't attempt to detect copy/move.
(Assignee)

Comment 11

4 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #9)

> I suspect this might be accidentally reverting 4c68f224513a.

Indeed, when I started those patches, bug 1143575 hadn't landed.
(Assignee)

Comment 12

4 years ago
Comment on attachment 8631459 [details] [diff] [review]
Add MP4 mediasource mochitests.

sigh... going to reclone with hg.
Attachment #8631459 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8631972 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Attachment #8631983 - Flags: review?(karlt)
(Assignee)

Comment 14

4 years ago
Attachment #8631984 - Flags: review?(karlt)
Attachment #8631983 - Flags: review?(karlt) → review+
Attachment #8631984 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/233db46481d7
https://hg.mozilla.org/mozilla-central/rev/40eefa29c76d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.