Open
Bug 1081814
Opened 10 years ago
Updated 11 months ago
Testcase for bug 1029552 : plays only one video on blinkx.com
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
NEW
People
(Reporter: bechen, Unassigned)
References
Details
Attachments
(1 file, 6 obsolete files)
3.17 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
Fork from bug 1029522 comment 25.
Comment 1•10 years ago
|
||
Hi, Benjamin
Sorry to bother you, could you help me to solve this bug?
My patch still can be successful on the mochitest.
The logic flow of my patch is that I created a video object, and I would change the source of it when it played to the end. If the current time of the object didn't equal to its duration when it ended, means that would happen a crash.
Thanks you a lot :)
Ps. The version I went back is "42f385becea7" which is just early before your patch.
Attachment #8508579 -
Flags: feedback?(bechen)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8508579 [details] [diff] [review]
patch_v1.patch
Review of attachment 8508579 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/test_bug1081814.html
@@ +12,5 @@
> +/*
> +var videoArr = ["huge-id3.mp3", "id3tags.mp3"];
> +*/
> +
> +var videoArr = ["gizmo.mp4", "gizmo.mp4"];
As JW says, did you try to add "?1 " at the end of url?
Attachment #8508579 -
Flags: feedback?(bechen)
Comment 3•10 years ago
|
||
(In reply to Benjamin Chen [:bechen] from comment #2)
> Comment on attachment 8508579 [details] [diff] [review]
> patch_v1.patch
>
> Review of attachment 8508579 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/media/test/test_bug1081814.html
> @@ +12,5 @@
> > +/*
> > +var videoArr = ["huge-id3.mp3", "id3tags.mp3"];
> > +*/
> > +
> > +var videoArr = ["gizmo.mp4", "gizmo.mp4"];
>
> As JW says, did you try to add "?1 " at the end of url?
I had tried it, but it didn't work.
JW said that the purpose of this command was used to create the individual cache for the object with same URL, then the system would regard them as the different object.
Did I miss something in my logic flow?
Thanks a lot :)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8508579 [details] [diff] [review]
patch_v1.patch
Review of attachment 8508579 [details] [diff] [review]:
-----------------------------------------------------------------
Please read bug 1029552 comment 6.
And about the url, I guess JW is right.
Or you can just pick two different video files to complete the testcase flow first, then try the url later.
::: content/media/test/test_bug1081814.html
@@ +38,5 @@
> + return;
> + }
> + info("Load video #" + videoIdx++);
> + v.preload = "auto";
> + v.src = videoArr.pop();
Here you do is "reset the src attribute" for the MediaElement.
In this testcase, we want to use a new MediaElement to replace the old one.
Updated•10 years ago
|
Assignee: nobody → alwu
Comment 5•10 years ago
|
||
Hi, Benjamin,
Could you help me review my patch?
Thanks a lot :)
Attachment #8508579 -
Attachment is obsolete: true
Attachment #8510005 -
Flags: review?(bechen)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8510005 [details] [diff] [review]
patch_v2.patch
Review of attachment 8510005 [details] [diff] [review]:
-----------------------------------------------------------------
Overall looks good to me.
::: content/media/test/test_bug1081814.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> + <title>Bug 1081814 - Testcase for bug 1029552 : plays only one video on blinkx.com </title>
We might need another description for the testcase and the file name. Something can describe the scenario or the code logic we want to test.
@@ +30,5 @@
> + var v = event.target;
> + //! The video plays fail if the current time doesn't change after time delay
> + setTimeout(function() {
> + info("Video #" + videoIdx +" Time = " + v.currentTime);
> + ok(v.currentTime != 0, "Play test : video #" + videoIdx + " is playing now");
Space.
@@ +38,5 @@
> + }
> +
> + /**
> + * Load video by creating a new object
> + */
Space.
@@ +50,5 @@
> + oldVideoObj = document.getElementsByTagName("video")[0];
> +
> + //! Create the video object
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
Space.
@@ +53,5 @@
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
> + var v = document.getElementsByTagName("video")[0];
> + v.addEventListener("play", checkVideoPlay, false);
> + v.addEventListener("ended", checkVideoEnd, false);
Space.
@@ +54,5 @@
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
> + var v = document.getElementsByTagName("video")[0];
> + v.addEventListener("play", checkVideoPlay, false);
> + v.addEventListener("ended", checkVideoEnd, false);
> + v.load();
I guess we don't need to call load() before play(). But it is ok.
@@ +57,5 @@
> + v.addEventListener("ended", checkVideoEnd, false);
> + v.load();
> + v.play();
> + }
> +
Extra line.
Attachment #8510005 -
Flags: review?(rlin)
Attachment #8510005 -
Flags: review?(bechen)
Attachment #8510005 -
Flags: review+
Updated•10 years ago
|
Attachment #8510005 -
Flags: review?(jwwang)
Comment 7•10 years ago
|
||
Comment on attachment 8510005 [details] [diff] [review]
patch_v2.patch
Review of attachment 8510005 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/test_bug1081814.html
@@ +28,5 @@
> +
> + function checkVideoPlay(event) {
> + var v = event.target;
> + //! The video plays fail if the current time doesn't change after time delay
> + setTimeout(function() {
setTimeout may not a good idea on try server... The timing isn't reliable.
@@ +40,5 @@
> + /**
> + * Load video by creating a new object
> + */
> + function loadVideo() {
> + if(videoArr.length===0) {
I think we don't need this array check.
@@ +46,5 @@
> + SimpleTest.finish();
> + return;
> + }
> + //! Avoid the previous object being release
> + oldVideoObj = document.getElementsByTagName("video")[0];
Could we use the getElementById?
Attachment #8510005 -
Flags: review?(rlin) → feedback+
Comment 8•10 years ago
|
||
Comment on attachment 8510005 [details] [diff] [review]
patch_v2.patch
Review of attachment 8510005 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/test_bug1081814.html
@@ +6,5 @@
> + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> + <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>
> +<div id = "playwrap"></div>
Per comment below, you don't need this div.
@@ +12,5 @@
> +
> +var videoIdx = 0;
> +var oldVideoObj;
> +var videoArr = [
> + '<video id="v-0" controls="controls" src="http://mochi.test:8888/tests/content/media/test/gizmo.mp4?1">Your browser does not support the video tag.</video>',
Can we just say |src="gizmo.mp4?1"|?
@@ +50,5 @@
> + oldVideoObj = document.getElementsByTagName("video")[0];
> +
> + //! Create the video object
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
You can put the video tags in the HTML code without creating them dynamically in the script.
@@ +54,5 @@
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
> + var v = document.getElementsByTagName("video")[0];
> + v.addEventListener("play", checkVideoPlay, false);
> + v.addEventListener("ended", checkVideoEnd, false);
> + v.load();
play() will trigger load(). You don't need load() here.
@@ +64,5 @@
> + loadVideo();
> +}
> +
> +SimpleTest.waitForExplicitFinish();
> +startTest();
Just call loadVideo(). You don't need the startTest() function.
Attachment #8510005 -
Flags: review?(jwwang) → review-
Comment 9•10 years ago
|
||
Comment on attachment 8510005 [details] [diff] [review]
patch_v2.patch
Review of attachment 8510005 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/test_bug1081814.html
@@ +25,5 @@
> + is(v.currentTime, v.duration, "End test : the video #" + videoIdx + " is ended normally");
> + loadVideo();
> + }
> +
> + function checkVideoPlay(event) {
You don't need this check. All you need is to verify the video can play till the end.
@@ +50,5 @@
> + oldVideoObj = document.getElementsByTagName("video")[0];
> +
> + //! Create the video object
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
Sorry, I just realized you clone the code from bug 1029552 comment 6. It is fine to stay with it now.
Comment 10•10 years ago
|
||
1. checkVideoPlay() & setTimeout()
Because the limitation of HW resource, the second video would went into a strange state after the play() was called.
It didn't get the end signal so that I couldn't test it in the checkVideoEnd().
For this reason, I tested it in checkVideoPlay() with setTimeout().
2. Asynchronous Tests
It was needed to add SimpleTest.finish() when waitForExplicitFinish() was added which used for the test case needed to wait for event.
Array length check is the check point where I call SimpleTest.finish() for end this test, another check point is in the checkVideoPlay().
3. getElementsByTagName() -> getElementById()
What is the reason for changing to this function? Is for readability or something else?
4.|src|
It would be fail when I wrote |src="gizmo.mp4?1"| in my patch.
Thanks a lot :)
Attachment #8510005 -
Attachment is obsolete: true
Attachment #8510053 -
Flags: review?(jwwang)
Comment 11•10 years ago
|
||
Comment on attachment 8510053 [details] [diff] [review]
p.patch
Review of attachment 8510053 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/test_bug1081814.html
@@ +5,5 @@
> +[Bug reason]
> +Video wouldn't be played because of the number limitation of hardware resource.
> +
> +[Test logic flow]
> +Step1. Prepare two video for playing
space.
@@ +24,5 @@
> +
> +var videoIdx = -1;
> +var oldVideoObj;
> +var videoArr = [
> + '<video id="v1" controls="controls" src="http://mochi.test:8888/tests/content/media/test/gizmo.mp4?1">Your browser does not support the video tag.</video>',
It works by saying |v.src = "gizmo.mp4?1";| in the script. So you can retrieve the video element in the script and assign "src" later.
@@ +30,5 @@
> +
> + /**
> + * The normal life cycle of video would be successful in both checkVideoPlay() and checkVideoEnd()
> + */
> + function checkVideoEnd(event) {
The indentation is wrong. It should be at the same level as |var oldVideoObj;| above.
@@ +58,5 @@
> + SimpleTest.finish();
> + return;
> + }
> + //! Avoid the previous object being release
> + oldVideoObj = document.getElementById("v" + videoIdx);
Why keeping the previous object?
@@ +63,5 @@
> +
> + //! Create the video object
> + info("Load video #" + ++videoIdx);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
> + var v = document.getElementById("v" + videoIdx);
I think you can do without videoIdx since there is only one child inside div.
@@ +70,5 @@
> + v.play();
> + }
> +
> +/**
> +* Program start
space.
Attachment #8510053 -
Flags: review?(jwwang)
Comment 12•10 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #10)
> 1. checkVideoPlay() & setTimeout()
> Because the limitation of HW resource, the second video would went into a
> strange state after the play() was called.
That is bug 1029552 intended to fix, right? So the test case shouldn't run into that situation.
Comment 13•10 years ago
|
||
Hi, JW,
It's my revision patch, could you help me review it?
Thanks a lot :)
Attachment #8510114 -
Flags: review?(jwwang)
Updated•10 years ago
|
Attachment #8510114 -
Flags: review?(jwwang) → review+
Comment 14•10 years ago
|
||
Attachment #8510053 -
Attachment is obsolete: true
Attachment #8510114 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
Comment on attachment 8510123 [details] [diff] [review]
patch_v5 [checkin patch]
Review of attachment 8510123 [details] [diff] [review]:
-----------------------------------------------------------------
I guess you got the wrong patch.
::: content/media/test/test_bug1081814.html
@@ +38,5 @@
> + }
> +
> + /**
> + * Load video by creating a new object
> + */
space
@@ +50,5 @@
> + oldVideoObj = document.getElementsByTagName("video")[0];
> +
> + //! Create the video object
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
space
@@ +53,5 @@
> + info("Load video #" + videoIdx++);
> + document.getElementById("playwrap").innerHTML = videoArr.pop();
> + var v = document.getElementsByTagName("video")[0];
> + v.addEventListener("play", checkVideoPlay, false);
> + v.addEventListener("ended", checkVideoEnd, false);
space
Attachment #8510123 -
Flags: review-
Comment 16•10 years ago
|
||
Hi, JW
Very sorry to make this stupid mistake, I would be more careful next time.
And I revised some part of my patch, could you help me review it?
Thanks a lots!
Attachment #8510123 -
Attachment is obsolete: true
Attachment #8510811 -
Flags: review?(jwwang)
Updated•10 years ago
|
Attachment #8510811 -
Flags: review?(jwwang) → review+
Comment 17•10 years ago
|
||
Hi, JW & Benjamin.
My patch is running fail on the XP and Android environment, the end event of the first video couldn't be trigger. [1]
So I try to write a simple test which is only with a embedded video tag, trying to check whether the video could be ended normally , but it still can't receive the end event. [2]
It means that we couldn't play a simple mpeg4 video in XP and Android? It's very weird. How do I fix it?
Thanks a lot :)
[1] Testcase is "bug1081814.html"
https://tbpl.mozilla.org/?tree=Try&rev=a6d6ab6c9427
[2] Testcase is "test_playmp4inxp.html "
https://tbpl.mozilla.org/?tree=Try&rev=13c361a31ebe
Flags: needinfo?(jwwang)
Flags: needinfo?(bechen)
Comment 18•10 years ago
|
||
Try to listen to the events listed here: http://dev.w3.org/html5/spec-preview/media-elements.html#mediaevents to see what events on earth come along.
Flags: needinfo?(jwwang)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bechen)
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Updated•7 years ago
|
Assignee: alastor0325 → nobody
Updated•2 years ago
|
Severity: normal → S3
Updated•11 months ago
|
Attachment #9385178 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•