929933 - Create a new HTML5 page with an audio/video file and player controls

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Infrastructure, defect, P2)

Description

[mario garbi]

We need this for our new test in Bug 851502. We only have nosound files on mozqa.com at the moment.

Comment 1

[mario garbi]

Attachment: newHTML5VideoPage.patch

Created the required page.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 820904 [details] [diff] [review]
newHTML5VideoPage.patch

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

This uses a remote video at http://videos-cdn.mozilla.net/uploads/marketing/firefox4/FF4_Jess3Features_VO_1.theora.ogv but this patch should instead contain any video files that are needed. Also, we only want our tests to use videos with no sound.

Comment 3

[mario garbi]

Unfortunately in order to reproduce the bug we need a HTML5 file with both video and audio channels as specified in comment 12 of initial Bug 834667. I have used the remote file as it was hosted on mozilla.net and did not have a clear download link, I figured it's not ok to grab it with a download tool.

Should I download the video file and add it to mozqa.com with my next patch? I wasn't yet able to reproduce the bug once with a video file that has no sound and managed to reproduce it every time with audio/video HTML5 files. If we cannot/do not want to use audio/video files this is something that can prevent the creation of the new test.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to mario garbi from comment #3)
> Should I download the video file and add it to mozqa.com with my next patch?
> I wasn't yet able to reproduce the bug once with a video file that has no
> sound and managed to reproduce it every time with audio/video HTML5 files.
> If we cannot/do not want to use audio/video files this is something that can
> prevent the creation of the new test.

No, we should request a suitable video file to be created as we have done before. Is it possible to have a silent or very low volume audio channel? We have a policy of no sound generated by our tests. In the past is have caused both confusion and annoyance.

Anthony: Do you recall who provided our previous video files?

Comment 5

[u279076]

IIRC it was Rainer Cvillink.

Comment 6

[mario garbi]

Hey Rainer, 

We need a html5 video file (.ogv) for an automated test we are developing and we only have nosound ones at the moment. It would have to be something that has an audio channel with minimal volume so that it won't bother the tester when it's run, about 1 minute long so that we can skip forward a couple of times and retrigger the buffer. 

Is this something you could help us with?

Comment 7

[Rainer Cvillink]

Hi there, here is the link to a video (ogv) about 2 min long with audio .... the audio is pretty low at -20bd .... let me know if that works for you guys ...

https://www.dropbox.com/s/cro8qpafhj0itmh/Testvideowithsound-20db.oggtheora.ogv

Comment 8

[mario garbi]

Thank you Rainer, the video is perfect and we can use it if Dave is ok with it, the issue reproduce it even if I mute the video using HTML5 controls. The only issue is it's size as it's too large to upload to bugzilla in a binary patch along with the html page.
Could you please trim it down to 1 minute? Is this something possible on your side as I do not have the necessary tools and expertise to properly do this.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The video looks fine to me. If you can still reproduce the video when reduced to 60 seconds, then that would be a good idea.

Comment 10

[Rainer Cvillink]

here is new video ... 60 sec and pretty small ... let me know if it works for you guys ...

https://www.dropbox.com/s/xzvryswxt80yb81/Video-for-QA.oggtheora.ogv

Comment 11

[mario garbi]

Thank you Rainer, the video is great, in fact it's perfect as we this we manage to finish the test and get the report instead of getting a Disconnect Error with "out of memory". 

I will come with a patch soon

Endurance report with high memory consumption(1000+):
http://mozmill-crowd.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa27866c96

Comment 12

[mario garbi]

Attachment: newHTML5VideoPage.patch

Updated the html5 file and added the video file Rainer provided to the patch. The problem reproduces with this clip muted as can be seen in the above comment.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to mario garbi from comment #11)
> Thank you Rainer, the video is great, in fact it's perfect as we this we
> manage to finish the test and get the report instead of getting a Disconnect
> Error with "out of memory". 
> 
> Endurance report with high memory consumption(1000+):
> http://mozmill-crowd.blargon7.com/#/endurance/report/
> d17690a112360a2b3155acaa27866c96

Oh wow. This is with a build which is affected by that bug, right? How does it look like with a more recent nightly build? I would like to see the difference.

Comment 14

[mario garbi]

The issue stopped after 17.0.02esrpre and here is the same build with 17.0.10esrpre where we can see it oscillates between 181-96 mb. We don't have this problem anymore with more recents builds or branches.

http://mozmill-crowd.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa27886cda

Comment 15

[mario garbi]

I meant the same test not the same build above, sorry for the typo.

Comment 16

[Rainer Cvillink]

cool ... let me know when you need that kind of stuff ... happy to provide it .....

Comment 17

[Andrei Eftimie]

Comment on attachment 828530 [details] [diff] [review]
newHTML5VideoPage.patch

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

This looks godo in principle, however I'd like to polish the html file a bit.
Sorry for being so nitpicky :)

Please leave a space after each ":" on all CSS declarations.

::: firefox/video/test_html5_video.html
@@ +3,5 @@
> +  <head>
> +    <style>
> +      .container {
> +        text-align:center;
> +        color:#06a2c3;

Set both text-align and color directly on the body

@@ +4,5 @@
> +    <style>
> +      .container {
> +        text-align:center;
> +        color:#06a2c3;
> +        margin:0 auto 0;

This doesn't do anything in this context.

@@ +6,5 @@
> +        text-align:center;
> +        color:#06a2c3;
> +        margin:0 auto 0;
> +        height:100%;
> +        width:100%;

Remove both height and width declarations.
- width is 100% for all block elements.
- height set to 100% has no effect in this case (it will try to match the height of the parent, and the parent does not have a defined height)

@@ +7,5 @@
> +        color:#06a2c3;
> +        margin:0 auto 0;
> +        height:100%;
> +        width:100%;
> +        padding-top:10;

This doesn't work right now. You would need to specify a unit (eg. 'px')

@@ +10,5 @@
> +        width:100%;
> +        padding-top:10;
> +      }
> +      .buttonbar {
> +        margin: 10 auto 10;

Specify a unit (eg. 'px'), otherwise this won't work.
'auto' does nothing without a fixed width.

You can write this as
margin: 10px 0;

@@ +15,5 @@
> +        text-align:center;
> +      }
> +    </style>
> +    <title>HTML5 Video File</title>
> +    <link rel="shortcut icon" type="image/ico" href="../images/mozilla_favicon.ico" />

1) You can remove the type.
2) rel="icon" should work fine
3) You don't need to self-close the tag. just ">" will suffice.

@@ +19,5 @@
> +    <link rel="shortcut icon" type="image/ico" href="../images/mozilla_favicon.ico" />
> +  </head>
> +
> +  <body>
> +    <div class="container">

We can also remove this div, we don't need it for the current design, just move the color and text-align on the body directly and you are set.

@@ +20,5 @@
> +  </head>
> +
> +  <body>
> +    <div class="container">
> +      <div>

Please remove this div, we don't need it.

@@ +21,5 @@
> +
> +  <body>
> +    <div class="container">
> +      <div>
> +        <h1> Desktop Automation - HTML5 Video File </h1>

Trim whitespace at the beginning and end of the title

@@ +25,5 @@
> +        <h1> Desktop Automation - HTML5 Video File </h1>
> +      </div>
> +
> +      <video id="videoPlayer">
> +        <source src="./Video-for-QA.oggtheora.ogv" type="video/ogg">

I think we can drop the "./" part

@@ +34,5 @@
> +        <button id="play" onclick="vidplay()">Play</button>
> +        <button id="fastFwd" onclick="skip(5)">&gt;&gt;</button>
> +      </div>
> +    </div>
> +    <script type="text/javascript">

You can remove the type, it is by default "text/javascript"

@@ +37,5 @@
> +    </div>
> +    <script type="text/javascript">
> +      function vidplay() {
> +        var video = document.getElementById("videoPlayer");
> +        var button = document.getElementById("play");

Move both declarations outside of the function.
We'll have them in the global namespace, but we shouldn't query for them each time we do an action.

@@ +38,5 @@
> +    <script type="text/javascript">
> +      function vidplay() {
> +        var video = document.getElementById("videoPlayer");
> +        var button = document.getElementById("play");
> +        video.muted = !video.muted;

Why are we resetting the muted attribute whenever we Pause the playback?
This should be set to false, and we should do that initially, not on ever play command.

@@ +48,5 @@
> +          button.textContent = "Play";
> +        }
> +      }
> +      function skip(value) {
> +        var video = document.getElementById("videoPlayer");

You can remove this when you'll move the other declarations in the global namespace.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 828530 [details] [diff] [review]
newHTML5VideoPage.patch

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

These HTML files should be minimal. We shouldn't need to add styles or unnecessary markup. Use the other files in firefox/video as reference. Also, the video file should be named similar to the others, such as sample-video-60s.ogv and the HTML should be named test_ogv_video_60s.html. We should probably also rename the existing HTML files to indicate the video length, but that can be done in a separate bug.

Comment 19

[mario garbi]

Attachment: newHTML5VideoPage_v2.patch

Thank you guys for the extensive review. I've made the changes suggested with this patch.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 830111 [details] [diff] [review]
newHTML5VideoPage_v2.patch

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

::: firefox/video/test_ogv_video_60s.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <title>HTML5 Video File</title>

Title should be in line with other files:

* OGV video via &lt;video&gt; (60s)

@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <title>HTML5 Video File</title>
> +    <link rel="icon" href="../images/mozilla_favicon.ico">

Why does this need a favicon?

@@ +6,5 @@
> +  </head>
> +
> +  <body>
> +    <video id="videoPlayer">
> +      <source src="sample-video-60s.ogv" type="video/ogg">

Can we use the src attribute of the video tag

@@ +9,5 @@
> +    <video id="videoPlayer">
> +      <source src="sample-video-60s.ogv" type="video/ogg">
> +    </video>
> +
> +    <div class="buttonbar">

We're not using styles, so no need for this class. In fact, we should just use the same links that we use in test_ogv_video_nosound.html but with the addition of a skip link and an input box for the value to skip by.

@@ +14,5 @@
> +      <button id="rew" onclick="skip(-5)">&lt;&lt;</button>
> +      <button id="play" onclick="vidplay()">Play</button>
> +      <button id="fastFwd" onclick="skip(5)">&gt;&gt;</button>
> +    </div>
> +    <script>

Please move this beneath <head> and create an init() function similar to the other video HTML files.

@@ +19,5 @@
> +      var video = document.getElementById("videoPlayer");
> +      var button = document.getElementById("play");
> +      video.muted = true;
> +      function vidplay() {
> +        if (video.paused) {

We shouldn't have this conditional logic as our tests should be deterministic.

Comment 21

[mario garbi]

Attachment: newHTML5VideoPage_v3.patch

Thank you Dave for the review, made the changes with this patch.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 830154 [details] [diff] [review]
newHTML5VideoPage_v3.patch

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

Thanks Mario, there are a few more small tweaks to make this consistent with the other video HTML files.

::: firefox/video/test_ogv_video_60s.html
@@ +21,5 @@
> +      function pause() {
> +        movie.pause();
> +      }
> +
> +      function stop() {

Is this necessary for the test? We could just use pause and seek for the same effect.

@@ +37,5 @@
> +    </script>
> +  </head>
> +
> +  <body onload="init();">
> +    <video id="videoPlayer" src="sample-video-60s.ogv" type="video/ogg"

Please add a <h1> tag as in the other video HTML files.

Is the type necessary? I notice we don't use it in test_ogv_video_nosound.html

Can you use an id of 'movie' to be consistent with the other file.

@@ +38,5 @@
> +  </head>
> +
> +  <body onload="init();">
> +    <video id="videoPlayer" src="sample-video-60s.ogv" type="video/ogg"
> +    controls="true"></video>

Are the controls necessary for the test?

@@ +46,5 @@
> +      <a href="#" onclick="stop()" id="stop">stop</a>
> +      <input type="number" id="seekField" value="0" />
> +      <a href="#" onclick="seek()" id="seek">seek</a>
> +      <input type="number" id="skipField" value="0" />
> +      <a href="#" onclick="skip()" id="skip">skip</a>

Can we include the current time as we do in test_ogv_video_nosound.html

Comment 23

[mario garbi]

Attachment: newHTML5VideoPage_v4.patch

Thank you Dave, I made the changes as requested, hope it's ok now.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 830826 [details] [diff] [review]
newHTML5VideoPage_v4.patch

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

Landed as:
https://hg.mozilla.org/qa/testcase-data/rev/b7c10c88a225

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Looks fine. But not sure if the 60s in the name were necessary.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Looks fine. But not sure if the 60s in the name were necessary.

This was to distinguish it from the existing files.