Closed Bug 448630 Opened 12 years ago Closed 10 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 3.7a5

People

(Reporter: info, Assigned: mozilla.bugs)

References

()

Details

Attachments

(1 file, 7 obsolete files)

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.
Version: unspecified → Trunk
Severity: normal → enhancement
OS: Windows XP → All
Hardware: PC → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
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? 



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).
Flags: in-litmus?
Attached patch wip (obsolete) — Splinter Review
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 ;)
Attachment #332286 - Attachment is obsolete: true
Priority: -- → P3
Target Milestone: --- → Firefox 3.2a1
Duplicate of this bug: 479153
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).
Duplicate of this bug: 486273
Blocks: 485686
Duplicate of this bug: 515888
Duplicate of this bug: 519317
Chris, are you working on a patch or shall we move this bug to new?
Target Milestone: Firefox 3.6a1 → ---
Chris/Cesar, is this under active development?  If not, I can have a go at it.
(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.
I'm not working on it - feel free to take it if you want it.
Assignee: chris.double → nobody
Status: ASSIGNED → NEW
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.
Assignee: nobody → mozilla.bugs
Status: NEW → ASSIGNED
Attached patch Patch v 1.0 (obsolete) — Splinter Review
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.
Attachment #357624 - Attachment is obsolete: true
Attachment #436821 - Flags: review?(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
Attached patch Patch v 1.1 (obsolete) — Splinter Review
Removed issues from your comments.
Attachment #436821 - Attachment is obsolete: true
Attachment #437060 - Flags: review?(dao)
Attachment #436821 - Flags: review?(dao)
Blocks: 377349
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.
(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.
Attached patch Patch v. 1.2 (obsolete) — Splinter Review
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.
Attachment #437060 - Attachment is obsolete: true
Attachment #438487 - Flags: review?(dao)
Attachment #437060 - Flags: review?(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.
(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.
(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;
Attachment #438487 - Flags: review?(dao) → review-
(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.
Attached patch Patch v. 1.3 (obsolete) — Splinter Review
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.
Attachment #438487 - Attachment is obsolete: true
Attachment #438810 - Flags: review?(dao)
One other item, do we want to set preload for the video and audio tags when Page Info > Media is opened?
Attached patch Patch v. 1.4 (obsolete) — Splinter Review
Attachment #438810 - Attachment is obsolete: true
Attachment #438982 - Flags: review+
Attachment #438810 - Flags: review?(dao)
Attachment #438982 - Flags: review+ → review?(dao)
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.
(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...
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.
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?
>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.
> 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.
Assignee: mozilla.bugs → nobody
Status: ASSIGNED → NEW
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.
(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.
Attached patch patch v1.5Splinter Review
s/src/currentSrc/ and some nits
Attachment #438982 - Attachment is obsolete: true
Attachment #439677 - Flags: review+
Attachment #438982 - Flags: review?(dao)
http://hg.mozilla.org/mozilla-central/rev/047529f5c096
Assignee: nobody → mozilla.bugs
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
Blocks: FF2SM
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?
(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."
Sorry for the confusion, but I was referring to the spelling only, not the context.
Sorry for the confusion, but I was referring to the spelling only, not the context.
(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...
Depends on: 560061
Just filed Bug 560061 with a simple patch awaiting Dao's review.
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 ;)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Verifying fixed using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100418 Minefield/3.7a5pre
Status: RESOLVED → VERIFIED
(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...
Depends on: 560258
Litmus testcase added:
https://litmus.mozilla.org/show_test.cgi?id=12903
Flags: in-litmus? → in-litmus+
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.