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)

Product:
Core
Component:
Audio/Video: Playback
Platform:
x86_64
Gonk (Firefox OS)
Type:
defect

Tracking

()

People

(Reporter: bechen, Unassigned)

References

Details

Attachments

(1 file, 6 obsolete files)

patch_v6
3.17 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
Fork from bug 1029522 comment 25.
Attached patch patch_v1.patch (obsolete) — — Splinter Review
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)
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)
(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 :)
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.
Assignee: nobody → alwu
Attached patch patch_v2.patch (obsolete) — — Splinter Review
Hi, Benjamin, Could you help me review my patch? Thanks a lot :)
Attachment #8508579 - Attachment is obsolete: true
Attachment #8510005 - Flags: review?(bechen)
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+
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 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 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.
Attached patch p.patch (obsolete) — — Splinter Review
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 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)
(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.
Attached patch patch_v4.patch (obsolete) — — Splinter Review
Hi, JW, It's my revision patch, could you help me review it? Thanks a lot :)
Attachment #8510114 - Flags: review?(jwwang)
Attachment #8510114 - Flags: review?(jwwang) → review+
Attached patch patch_v5 [checkin patch] (obsolete) — — Splinter Review
Attachment #8510053 - Attachment is obsolete: true
Attachment #8510114 - Attachment is obsolete: true
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-
Attached patch patch_v6 — — Splinter Review
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)
Attachment #8510811 - Flags: review?(jwwang) → review+
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)
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)
Flags: needinfo?(bechen)
Component: Audio/Video → Audio/Video: Playback
Assignee: alastor0325 → nobody
Severity: normal → S3
Attachment #9385178 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: