448630 - new <audio> and <video> tags don't show up in Tools > Page Info > Media

Status: VERIFIED FIXED

(Firefox :: Page Info Window, enhancement, P3)

Description

[skierpage]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008073003 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008073003 Minefield/3.1a2pre

See summary

Reproducible: Always

Steps to Reproduce:
1. Go to a page using the cool new <audio> or <video> tags.
2. Choose Tools > Page Info
Actual Results:  
If there is no other media on the page there's no Media tab in Page Info.  If there is other media the Media Tab doesn't list the audio and video tags' contents.

Expected Results:  
At least list the address and type in the Page Info Media tab.  For bonus points, preview with basic controls.

Comment 1

[cajbir (:cajbir)]

Attachment: First cut at displaying video/audio details in page info

This is a first cut at adding video/audio details in the media tab of page info. It doesn't do preview, just lists the details, at this stage.

Comment 2

[Graeme McCutcheon [:graememcc]]

Had been considering having a look at this, but Chris beat me to it!

Should we rename addImage to addMedia?

Rather than a full playable preview, if the metadata is loaded, should we show the poster image if it exists? 

Comment 3

[cajbir (:cajbir)]

Feel free to finish it! I have plenty of others to keep me busy if you'd like to have a go. 

Showing the poster image would be a good alternative if the user supplied a poster attribute (although that'll require bug 449156 to be done). Getting the current frame of the video might even be possible if there is no poster attribute (see bug 448674).

Comment 4

[u278084]

Attachment: wip

An updated version of the first cut. Uses canvas' drawImage to get the current frame of the video. I think this will be a good fallback method if poster isn't specified.

Missing audio image. Wondering what to put here. I could draw my own image, but that will leave much to be desired ;)

Comment 6

[Jo Hermans]

http://www.0xdeadbeef.com/weblog/?p=1173

Strangely enough, Page Info lists the 'embed' media (fallback for the video), even though it's never read in memory (that's why it displays grey).

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Chris, are you working on a patch or shall we move this bug to new?

Comment 11

[Tanner M. Young [:tmyoung]]

Chris/Cesar, is this under active development?  If not, I can have a go at it.

Comment 12

[Tanner M. Young [:tmyoung]]

(In reply to comment #4)
> Missing audio image. Wondering what to put here. I could draw my own image, but
> that will leave much to be desired ;)

It would be nice, but I don't think we need one or that it would block this bug by not having one.

Comment 13

[cajbir (:cajbir)]

I'm not working on it - feel free to take it if you want it.

Comment 14

[Tanner M. Young [:tmyoung]]

I am in the middle of another Page Info patch that will occupy me for another day or two; once I'm done with it, I will move on this.

Comment 15

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.0

This takes the WIP and adds the ogg audio and video files as elements to the preview area so that the user can listen to or watch the media, respectively.

I have prevented dimensions from appearing on audio but allowed them on video, hence the isAudio boolean for makePreview.

I have added the html namespace to Page Info to allow the video elements to play.  Audio elements are taken care of by the Audio() function.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 436821 [details] [diff] [review]
Patch v 1.0

>+  else if (item instanceof HTMLVideoElement) {
>+    var newImage = document.createElementNS("http://www.w3.org/1999/xhtml", "html:video");

s/html:video/video/

>--- a/browser/base/content/pageinfo/pageInfo.xul
>+++ b/browser/base/content/pageinfo/pageInfo.xul
>@@ -51,16 +51,17 @@
> ]>
> 
> #ifdef XP_MACOSX
> <?xul-overlay href="chrome://browser/content/macBrowserOverlay.xul"?>
> #endif
> 
> <window id="main-window"
>   xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+  xmlns:html="http://www.w3.org/1999/xhtml"
>   windowtype="Browser:page-info"
>   onload="onLoadPageInfo()"
>   onunload="onUnloadPageInfo()"
>   align="stretch"
>   screenX="10" screenY="10"
>   width="&pageInfoWindow.width;" height="&pageInfoWindow.height;"
>   persist="screenX screenY width height sizemode">

Seems superfluous

Comment 17

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.1

Removed issues from your comments.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 437060 [details] [diff] [review]
Patch v 1.1

>+#ifdef MOZ_MEDIA
>+  else if (item instanceof HTMLVideoElement) {
>+    var newImage = document.createElementNS("http://www.w3.org/1999/xhtml", "video");

newImage is already declared.

>+    newImage.setAttribute("id", "thepreviewimage");
>+    var physWidth = 0, physHeight = 0;
>+    var width = 0, height = 0;

s/var/let/

>+    newImage.setAttribute("src", url);
>+    newImage.width = item.videoWidth;
>+    newImage.height = item.videoHeight;
>+    newImage.controls = "controls";
>+    physWidth = newImage.width || 0;
>+    physHeight = newImage.height || 0;
>+    width = newImage.width;
>+    height = newImage.height;

Can you use mozLoadFrom to copy the possibly already-loaded data?

>+    document.getElementById("theimagecontainer").collapsed = false;
>+    document.getElementById("brokenimagecontainer").collapsed = true;
>+  }
>+  else if (item instanceof HTMLAudioElement) {
>+    var newImage = new Audio();

newImage is already declared.

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> >+    newImage.setAttribute("id", "thepreviewimage");
> >+    var physWidth = 0, physHeight = 0;
> >+    var width = 0, height = 0;
> 
> s/var/let/

Actually, width and height are declared already too.

Comment 20

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 1.2

Using mozLoadFrom() removes the need for the src check and links this video to the cache for its counterpart in the page, but the height and width checks still need to be done manually, since mozLoadFrom doesn't deal with size.  Setting the height and width in here

> + newImage.width = item.videoWidth;
> + newImage.height = item.videoHeight;

Allows for Page Info to know what the height and width of the video are without touching the imageSize declaration.

I have removed the extra variables and variable declarations and made the other changes from the last comments.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> the height and width checks
> still need to be done manually, since mozLoadFrom doesn't deal with size.

What needs to be dealt with there?

> Setting the height and width in here
> 
> > + newImage.width = item.videoWidth;
> > + newImage.height = item.videoHeight;
> 
> Allows for Page Info to know what the height and width of the video are without
> touching the imageSize declaration.

I don't understand what you mean.

Comment 22

[Tanner M. Young [:tmyoung]]

(In reply to comment #21)
> (In reply to comment #20)
> > the height and width checks
> > still need to be done manually, since mozLoadFrom doesn't deal with size.
> 
> What needs to be dealt with there?

The height and width do not inherit if I use mozLoadFrom.

> 
> > Setting the height and width in here
> > 
> > > + newImage.width = item.videoWidth;
> > > + newImage.height = item.videoHeight;
> > 
> > Allows for Page Info to know what the height and width of the video are without
> > touching the imageSize declaration.
> 
> I don't understand what you mean.

If I remove those lines of code listed above, then the page info window lists the dimensions as being -1px x -1px.

The long and the short being, we can add mozLoadFrom by all means, but we still need all of the code related to height and width, or it will not show the video or it will not list the correct dimensions.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > the height and width checks
> > > still need to be done manually, since mozLoadFrom doesn't deal with size.
> > 
> > What needs to be dealt with there?
> 
> The height and width do not inherit if I use mozLoadFrom.
> 
> > 
> > > Setting the height and width in here
> > > 
> > > > + newImage.width = item.videoWidth;
> > > > + newImage.height = item.videoHeight;
> > > 
> > > Allows for Page Info to know what the height and width of the video are without
> > > touching the imageSize declaration.
> > 
> > I don't understand what you mean.
> 
> If I remove those lines of code listed above, then the page info window lists
> the dimensions as being -1px x -1px.

Because you're doing this:

+    newImage.width = item.videoWidth;
+    newImage.height = item.videoHeight;
+    physWidth = newImage.width || 0;
+    physHeight = newImage.height || 0;
+    width = newImage.width;
+    height = newImage.height;

You probably want to do something like this instead:

width = physWidth = item.videoWidth;
height = physHeight = item.videoHeight;

Comment 24

[Tanner M. Young [:tmyoung]]

(In reply to comment #23)
> width = physWidth = item.videoWidth;
> height = physHeight = item.videoHeight;

That works for me, but it brings up two thoughts on my part.

First, the video size is not defined until the video is streamed that information.  I personally would prefer the video size to be defined when the object is selected in Page Info windows since the browser window already has that information.

Second--which is a larger issue--the height and width of the video can be changed manually on the browser page with the height and width attributes in the video.  To be consistent with the image resizing in Page Info, we should also resize the video to its size on the original page.

To take care of both of these, the patch can use the physWidh/physHeight for the item.videoWidth/item.videoHeight, and then use height and width for the height and width attributes from the tag if they have been defined, or something to that effect.  This would make the behavior the same for videos and images as far as the resizing question is concerned.

Comment 25

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 1.3

This addresses Dao's concerns and mine.

This patch checks if the item.width/item.height is defined, and if it is and it's set to something, then the video is resized to that size.  If it is not, then there the video defaults to the item.videoHeight/item.videoWidth.

Comment 26

[Tanner M. Young [:tmyoung]]

One other item, do we want to set preload for the video and audio tags when Page Info > Media is opened?

Comment 27

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 1.4

Comment 28

[Tanner M. Young [:tmyoung]]

Comment on attachment 438982 [details] [diff] [review]
Patch v. 1.4

I just noticed that the patch from before doesn't make use of the isProtocolAllowed check, which it should for audio and video tags.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #24)
> To be consistent with the image resizing in Page Info, we should
> also resize the video to its size on the original page.

That seems strange. I'd expect images to show up at their original size, just like "View Image" from the context menu does it. They are usually resized to fit the page layout, but inheriting that to the media pane doesn't make sense.

(In reply to comment #26)
> One other item, do we want to set preload for the video and audio tags when
> Page Info > Media is opened?

No...

Comment 30

[Alex Faaborg [:faaborg] (Firefox UX)]

I was pinged on irc to provide a ux view on this bug.  Sorry for my lack of comprehension here, but can anyone answer what the main use case is for the media panel.

Comment 31

[Tanner M. Young [:tmyoung]]

Speaking from my own knowledge and experience, this is primarily a developer tool or an advanced user tool, and therefore, probably not something that most Firefox users see on a daily basis.  It can be used to see all images and background images on a given page (with audio and video naturally in the works), and it is accessed through "Tools > Page Info > Media" or "Context Menu > Page Info > Media" and the "Context Menu > View Image Info" which goes directly to the media panel and shows the clicked image.

Does that help?

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

>this is primarily a developer tool or an advanced user tool

Right, so I definitely get that, what I don't understand is exactly what the developer wants to accomplish by using this tool.  Seeing all the images/background images/audio/video is what the tool does, but not necessarily what a developer needs to do.  It seems like web developers would be better served by more powerful tools (like the net tab in Firebug), and mainstream users who want to archive large amounts of media from the Web would be best served by an extension.

Comment 33

[Tanner M. Young [:tmyoung]]

> It seems like web developers would be better served by more powerful tools

That's the deeper question that needs to be answered at some point.

Regardless of whether we decide to remove the media panel or change anything about this patch, I'm afraid I can't devote any more time to this, so I am unassigning myself from the bug.

My thought is that if the patch is good, then let's commit it, and we can address the larger issue of the media panel later, since that could occur anytime before 3.7 is released which is still some time away.  However, if there any changes that ui-review or code review require, I wont be able to make those for some time; so, in the interest of the community, I'll happily let someone else finish this if they are so inclined.

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

Ok, sorry to derail the bug but I agree that we should figure out what we want to do with the media panel before we invest too much time in making changes to it.  We can of course still take the patch if it gets a review.  Thanks for pinging someone from the ux team for feedback, really appreciate it.

Comment 35

[Tanner M. Young [:tmyoung]]

(In reply to comment #34)
> Ok, sorry to derail the bug but I agree that we should figure out what we want
> to do with the media panel before we invest too much time in making changes to
> it.  We can of course still take the patch if it gets a review.  Thanks for
> pinging someone from the ux team for feedback, really appreciate it.

Oh, you haven't derailed anything.  I've actually been waiting for someone to ask this question in UX, since there has already been a fair amount of debate about the Page Info in general.  I just wouldn't have time to remove it or do more than a one-line fix in this patch because of other responsibilities I have.

Comment 36

[Dão Gottwald [:dao]]

Attachment: patch v1.5

s/src/currentSrc/ and some nits

Comment 37

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/047529f5c096

Comment 38

[Daniel C]

Comment on attachment 439677 [details] [diff] [review]
patch v1.5


>+  var isAudo = false;

>+    isAudo = true;

>-  if (url) {
>+  if (url && !isAudo) {

This should be isAudio, right?

Comment 39

[Tanner M. Young [:tmyoung]]

(In reply to comment #38)
> (From update of attachment 439677 [details] [diff] [review])
> This should be isAudio, right?

No, isAudio is designed to prevent the dimensions from being listed on audio tags.  If that boolean is switched, dimensions would be turned off for images/videos and shown with useless/broken data only on the audio tags.

So that line states, "if this tag has a url and is not an audio tag, then show the dimension information."

Comment 40

[Daniel C]

Sorry for the confusion, but I was referring to the spelling only, not the context.

Comment 41

[Daniel C]

Sorry for the confusion, but I was referring to the spelling only, not the context.

Comment 42

[Tanner M. Young [:tmyoung]]

(In reply to comment #41)
> Sorry for the confusion, but I was referring to the spelling only, not the
> context.

Yes, it probably should reflect what it is in English...

Comment 43

[Tanner M. Young [:tmyoung]]

Just filed Bug 560061 with a simple patch awaiting Dao's review.

Comment 44

[Jo Hermans]

(In reply to comment #24)
> First, the video size is not defined until the video is streamed that
> information.

Is that also the reason why the size of the video in <http://weblogs.mozillazine.org/gerv/archives/2010/03/why_theology_is_important.html> is reported to be 11.46KB ? Size of preview image I guess.

Comment 45

[u88484]

Nothing shows up in the media panel for the URL listed in this bug report.  Only one .jpg image is listed although the page has both <audio> and <video> on the page.

Comment 46

[u88484]

(In reply to comment #45)
> Nothing shows up in the media panel for the URL listed in this bug report. 
> Only one .jpg image is listed although the page has both <audio> and <video> on
> the page.

Helps if I actually restart after updating ;)

Comment 47

[u88484]

Verifying fixed using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100418 Minefield/3.7a5pre

Comment 48

[Tanner M. Young [:tmyoung]]

(In reply to comment #44)
> Is that also the reason why the size of the video in
> <http://weblogs.mozillazine.org/gerv/archives/2010/03/why_theology_is_important.html>
> is reported to be 11.46KB ? Size of preview image I guess.

I get a different size each time I reload the page, which would seem to indicate that for this video whatever is in the cache when Page Info loads is what is listed as the size.  I really don't know if that is flaw or a feature...

Comment 49

[u279076]

Litmus testcase added:
https://litmus.mozilla.org/show_test.cgi?id=12903