Last Comment Bug 557278 - OGG audio handled as OGV video
: OGG audio handled as OGV video
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- major (vote)
: Firefox 14
Assigned To: Paul Adenot (:padenot)
:
Mentors:
http://ct1.party107.com:8000/Party107...
: 480031 497666 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-05 11:27 PDT by Luis Villa [Outside Counsel; for non-law use luis@tieguy.org]
Modified: 2012-04-23 11:48 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 - Better handling of media (audio/video) saving (3.94 KB, patch)
2011-06-15 11:41 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Patch v1 - Better handling of media (audio/video) saving (4.75 KB, patch)
2011-06-16 07:14 PDT, Paul Adenot (:padenot)
dolske: review-
dolske: feedback+
Details | Diff | Review
Patch v3 - Addressed dolke concerns (3.56 KB, patch)
2011-07-06 06:06 PDT, Paul Adenot (:padenot)
dolske: review+
Details | Diff | Review
Rebased and updated the test. (70.42 KB, patch)
2012-04-17 11:27 PDT, Paul Adenot (:padenot)
dolske: review+
Details | Diff | Review

Description Luis Villa [Outside Counsel; for non-law use luis@tieguy.org] 2010-04-05 11:27:43 PDT
When I open an audio stream (e.g http://ct1.party107.com:8000/Party107-Q0.ogg ) and then right-click on the player, I get options like:
'view video', 'copy video location', etc. Those should be 'play audio', 'copy audio location', 'save audio', 'send audio', etc.

This is mostly merely cosmetic, but note that when people click 'view video' and then get no video (which they will do) some of them will assume the stream contains video and blame us for not playing it.
Comment 1 Luis Villa [Outside Counsel; for non-law use luis@tieguy.org] 2010-06-30 10:41:47 PDT
This is actually substantially worse than I originally thought. Open an ogg audio file on the web (such as the one in the URL field). Not only does the right-click menu say 'save video as', it actually changes the file extension of the file, which is going to break a fair number of things when people try to reopen it later.

In other words, when I right click on the audio file above, this is what happens:

* says 'save video as'
* saves it as Party107-Q0.ogg.ogv

It should:

* say 'save audio as'
* save it as Party107-Q0.ogg (no new, and incorrect, .ogv extension.)
Comment 2 Luis Villa [Outside Counsel; for non-law use luis@tieguy.org] 2011-05-20 12:52:37 PDT
Poke? Still an issue in 6.0a.
Comment 3 Paul Adenot (:padenot) 2011-06-15 06:48:54 PDT
I still have this issue on today nightly.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-06-15 06:56:14 PDT
Some more info on this bug...Right click the page, select Page Info > Media...the metadata for the test file shows that it is a video. 

Using a different source: http://www.vorbis.com/music/

It appears that Firefox detects these as videos as well. I think the issue here isn't that Firefox is saving audio as video, it's that Firefox is detecting/handling .ogg audio files as video from the get-go.
Comment 5 Paul Adenot (:padenot) 2011-06-15 11:41:30 PDT
Created attachment 539606 [details] [diff] [review]
Patch v1 - Better handling of media (audio/video) saving

Here is a patch that address the problem for the saving part. It's not finished, though.

The thing is that the mime type for the media is application/ogg, and this could be audio or video. And sometime, when viewing an ogg video, it is served with a audio/ogg MIME type (example : http://ftp.akl.lt/Video/Big_Buck_Bunny/big_buck_bunny_720p_stereo.ogg).

As no content type is provided when saving a media, relies on the system to find what extension it should use for the file name, and it was deducing video/ogg.

I had the same problem saving from an audio tag.

Specifically, we should not display a video frame when we are only playing audio (a quick way to do that would be to check the video dimension in the metadata), and treat the media as audio (for the context menus).

I'm going to address the other issue we encounter in some other patch.
Comment 6 Paul Adenot (:padenot) 2011-06-16 07:14:26 PDT
Created attachment 539790 [details] [diff] [review]
Patch v1 - Better handling of media (audio/video) saving

I've added audio / video detection for the context menu when a ogg file is loaded without an audio tag. I've tested quite a few cases, and it seem to work quite well (and it even works when the media is served with the wrong mimetype).
Comment 7 Paul Adenot (:padenot) 2011-06-17 07:36:56 PDT
I've tried to dig a little deeper to fix the "audio treated as a video" problem as its root source, and I face a problem.

In content/html/document/src/VideoDocument.cpp, when the fake <video> element is instantiated, we cannot know if we need a video or an audio element (if it is ogg served with an application/ogg, it can be both video and audio only, and we haven't got data to choose), so we are not able to instantiate an <audio> element instead. The <video> element must then mimick an <audio> element, by changing the context menu (as it is currently done), and by finding a way not to draw the video rendering frame. But this is probably a nitpick.
Comment 8 Paul Adenot (:padenot) 2011-06-23 07:23:48 PDT
Comment on attachment 539790 [details] [diff] [review]
Patch v1 - Better handling of media (audio/video) saving

Justin, according to "hg annotate -u", you seem to be the person that have modified this file the most. Feel free to point me to a more appropriate person if you can't/don't want to do this review.
Comment 9 antistress 2011-06-26 16:27:23 PDT
On a related note, bug 619421 describes that <audio controls> progress UI should not be the same that the <video controls> progress UI
Comment 10 antistress 2011-06-26 16:36:30 PDT
Maybe this is a dup : Bug 480031 - Context menu for audio files should not have a "View Video" option
Comment 11 Paul Adenot (:padenot) 2011-06-27 02:43:40 PDT
*** Bug 480031 has been marked as a duplicate of this bug. ***
Comment 12 Justin Dolske [:Dolske] 2011-07-05 18:56:57 PDT
Comment on attachment 539790 [details] [diff] [review]
Patch v1 - Better handling of media (audio/video) saving

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

Sorry for all of today's review grilling. ;-)

General approach seems fine, but r- for the details.

::: browser/base/content/nsContextMenu.js
@@ +489,5 @@
>        }
>        else if (this.target instanceof HTMLVideoElement) {
> +        // Firefox always creates a HTMLVideoElement when it reads an ogg file.
> +        // If the media is actually audio, be smarter and provide a context menu
> +        // with audio operations.

That's not quite right, it's only <video> when loaded directly (ie, a VideoDocument). It would be preferable to only look for audio-as-video when explicitly in that case, but I'm not sure how. Hence bug 462892. :)

@@ +494,5 @@
> +        if (this.target.videoWidth == 0 || this.target.videoHeight == 0) {
> +          this.onAudio = true;
> +        } else {
> +          this.onVideo = true;
> +        }

Nit: Local style here seems to be to skip braces inless one block is multi-line.

@@ +1041,5 @@
>        urlSecurityCheck(this.mediaURL, doc.nodePrincipal);
>        var dialogTitle = this.onVideo ? "SaveVideoTitle" : "SaveAudioTitle";
> +      var reg = /.*\.ogg/;
> +      var type = null;
> +      if (this.target.src.match(reg) || this.target.src.length == 0) {

Regex is too loose, instead use: /\.ogg$/i 

Better to use |reg.test(this.mediaURL)| here, since target.src isn't always what you want (loading a new media source can be done without changing the src attribute).

Hmm, why is this even OGG specific in the first place? Can't WebM have the same problem?

@@ +1043,5 @@
> +      var reg = /.*\.ogg/;
> +      var type = null;
> +      if (this.target.src.match(reg) || this.target.src.length == 0) {
> +        var type = this.onVideo ? "video/ogg" : "audio/ogg";
> +        if (this.target.videoWidth == 0 || this.target.videoHeight == 0) {

Seems like it would be safer to only set the |type| override for the special case mentioned above.

@@ +1045,5 @@
> +      if (this.target.src.match(reg) || this.target.src.length == 0) {
> +        var type = this.onVideo ? "video/ogg" : "audio/ogg";
> +        if (this.target.videoWidth == 0 || this.target.videoHeight == 0) {
> +          type = "audio/ogg";
> +          dialogTitle = "SaveAudioTitle";

I don't think this is needed? With the change earlier in the patch, this.onAudio would already be true, and have the correct dialog title.

::: toolkit/content/contentAreaUtils.js
@@ +536,5 @@
> +    // If we have the extension as well, compute the basename and return
> +    if (aFI.fileName && aFI.fileExt) {
> +      aFI.fileBaseName = getFileBaseName(aFI.fileName);
> +      return;
> +    }

What's this change for?
Comment 13 Paul Adenot (:padenot) 2011-07-06 06:06:33 PDT
Created attachment 544218 [details] [diff] [review]
Patch v3 - Addressed dolke concerns

> Hmm, why is this even OGG specific in the first place? Can't WebM have the same > problem?

WebM cannot (as far as I know) contains only audio, so the MIME type check works, without having to rely on other checks.

Also, I've removed quite a few tests, and added a |readyState >= 1| check when we are testing against metadata.

> What's this change for?
Nothing, I can't even remember why I wrote that in the first place.
Comment 15 Paul Adenot (:padenot) 2011-07-06 09:11:57 PDT
Oh, I didn't event know that .weba possible, thank you for pointing that out.
I'll update my patch tomorrow.
Comment 16 antistress 2011-07-06 09:26:26 PDT
(In reply to comment #15)
> Oh, I didn't event know that .weba possible, thank you for pointing that out.
> I'll update my patch tomorrow.

It's not that clear to me

There are still some discussions about the opportunity of having audio only for WebM as far as i understand
Comment 17 Timothy B. Terriberry (:derf) 2011-07-06 09:33:55 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Oh, I didn't event know that .weba possible, thank you for pointing that out.
> > I'll update my patch tomorrow.
> 
> It's not that clear to me
> 
> There are still some discussions about the opportunity of having audio only
> for WebM as far as i understand

We support the audio/webm mimetype and don't do anything to make playback fail if there's no video stream, so something at least is possible. Whether it's a good idea (audio-only WebM provides no additional features over plain Vorbis-in-Ogg, and breaks compatibility with over a decade's worth of existing software and hardware deployment) is a separate question.
Comment 18 Justin Dolske [:Dolske] 2011-07-06 19:28:58 PDT
Comment on attachment 544218 [details] [diff] [review]
Patch v3 - Addressed dolke concerns

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

Bonus points if you add a test for the audio-in-video case... See base/content/test/test_contextmenu.html, should just need to add a case for <video src="audio.ogg">

::: browser/base/content/nsContextMenu.js
@@ +509,5 @@
>        else if (this.target instanceof HTMLVideoElement) {
> +        // Firefox always creates a HTMLVideoElement when loading an ogg file
> +        // directly. If the media is actually audio, be smarter and provide a context menu
> +        // with audio operations.
> +        if (this.target.readyState >= 1 && (this.target.videoWidth == 0 || this.target.videoHeight == 0))

The .readyState check is a good idea. Maybe we should have videocontrols.xml do similar...

Would slightly prefer to check .readyState >= .HAVE_METADATA.
Comment 19 Justin Dolske [:Dolske] 2011-07-06 19:35:28 PDT
Hmm, I wonder if webm/weba is actually likely to have this problem. OGG is a bit of a special case, because it seems people commonly just use ".ogg" for both, instead of .ogv/.oga. Maybe we should wait to see if people actually start using ".webm" for audio-only content before giving it the same treatment. With this current patch, I think <video src="foo.weba"> should correctly offer "Save Audio As" and not result in foo.weba.webm.
Comment 20 Paul Adenot (:padenot) 2011-07-08 03:47:24 PDT
After testing the behavior using both .weba and .webm extension, with this patch applied, it goes like this, considering a file with no audio information :

- If the media ends in .weba, in both <audio> and <video> tags, the right click context menu show "Save audio", as expected. This menu has "Save audio" as title, and the file type filter shows "All file", as is the extension was not recognized.
- If the media ends in .webm, everything works (context menu, save file window title), but the file type filter shows "Web Media Video".

In all cases the file names are correct (i.e. are respecting the original src attribute value).

(test page here : http://paul.cx/test/webm.html)

Justin, I'll write a test for this, and address your last comments.
Comment 21 Paul Adenot (:padenot) 2011-07-11 07:49:32 PDT
I pushed to try quite a few patches, including this one, its test, and a other patch for this test, since we add a context menu in bug 559468.

http://tbpl.mozilla.org/?tree=Try&rev=6e79653c0032
Comment 22 Paul Adenot (:padenot) 2011-07-11 08:39:13 PDT
Of course it couldn't work on the first try. Here is the right push :
http://tbpl.mozilla.org/?tree=Try&rev=765d42fb098b
Comment 23 Paul Adenot (:padenot) 2011-07-21 09:27:34 PDT
*** Bug 497666 has been marked as a duplicate of this bug. ***
Comment 24 Paul Adenot (:padenot) 2012-04-17 11:27:10 PDT
Created attachment 615803 [details] [diff] [review]
Rebased and updated the test.

Just figured out this old patch had not been landed. I rebased it and updated the test.
Comment 25 Mozilla RelEng Bot 2012-04-17 14:19:09 PDT
Autoland Patchset:
	Patches: 615803
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=cfaf60a6a6d7
Try run started, revision cfaf60a6a6d7. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=cfaf60a6a6d7
Comment 26 Mozilla RelEng Bot 2012-04-17 14:31:01 PDT
Autoland Patchset:
	Patches: 615803
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=74e69d6867d8
Try run started, revision 74e69d6867d8. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=74e69d6867d8
Comment 27 Mozilla RelEng Bot 2012-04-17 15:48:30 PDT
Try run for cfaf60a6a6d7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cfaf60a6a6d7
Results (out of 15 total builds):
    exception: 14
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-cfaf60a6a6d7
Comment 28 Ralph Giles (:rillian) needinfo me 2012-04-17 16:24:12 PDT
14*"Infrastructure exception"

Trying try again...
Comment 29 Mozilla RelEng Bot 2012-04-17 20:45:20 PDT
Try run for 74e69d6867d8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=74e69d6867d8
Results (out of 232 total builds):
    success: 203
    warnings: 29
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-74e69d6867d8
Comment 30 Ralph Giles (:rillian) needinfo me 2012-04-17 22:27:47 PDT
That is, two known orange, and a bunch of interrupted jobs which later completed.
Comment 31 Justin Dolske [:Dolske] 2012-04-19 23:55:36 PDT
(Nice to see you in video bugs again! :)
Comment 32 Paul Adenot (:padenot) 2012-04-20 09:48:12 PDT
Thanks for the review.

Note that you can probably see me in real life as well, I'm less than 10 meters from your desk :-)
Comment 33 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-20 15:25:23 PDT
Comment on attachment 615803 [details] [diff] [review]
Rebased and updated the test.

[Triage comment]
/browser has a=desktop-only clearance to land.
Comment 34 Ryan VanderMeulen [:RyanVM] 2012-04-20 19:36:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b66f3caf2ca
Comment 35 Phil Ringnalda (:philor) 2012-04-21 23:50:44 PDT
https://hg.mozilla.org/mozilla-central/rev/1b66f3caf2ca

Note You need to log in before you can comment on or make changes to this bug.