Closed Bug 1148103 Opened 9 years ago Closed 9 years ago

Convert all MediaSource mochitests to use mp4 content

Categories

(Core :: Audio/Video, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Priority: -- → P3
Assignee: nobody → jyavenard
Attached patch Add MP4 mediasource mochitests. (obsolete) — Splinter Review
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-
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.
(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.
Attached patch Clone files (obsolete) — Splinter Review
Attached patch Add MP4 mediasource mochitests. (obsolete) — Splinter Review
This only replicates the webm tests using mp4 content.
Attachment #8631459 - Flags: review?(karlt)
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-
(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.
(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.
Comment on attachment 8631459 [details] [diff] [review]
Add MP4 mediasource mochitests.

sigh... going to reclone with hg.
Attachment #8631459 - Attachment is obsolete: true
Attachment #8631972 - Attachment is obsolete: true
Attachment #8631983 - Flags: review?(karlt)
Attachment #8631984 - Flags: review?(karlt)
Attachment #8631983 - Flags: review?(karlt) → review+
Attachment #8631984 - Flags: review?(karlt) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: