Closed Bug 557278 Opened 14 years ago Closed 12 years ago

OGG audio handled as OGV video

Categories

(Firefox :: Menus, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: villalu, Assigned: padenot)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
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.)
Severity: minor → major
Summary: right-click menu refers to 'video' even when the content is an audio stream → 'save video as' breaks file names by adding .ogv to .ogg *audio* file
Poke? Still an issue in 6.0a.
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Mac OS X → All
I still have this issue on today nightly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Hardware: x86 → All
Summary: 'save video as' breaks file names by adding .ogv to .ogg *audio* file → OGG audio handled as OGV video
Assignee: nobody → paul
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.
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).
Attachment #539606 - Attachment is obsolete: true
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 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.
Attachment #539790 - Flags: feedback?(dolske)
On a related note, bug 619421 describes that <audio controls> progress UI should not be the same that the <video controls> progress UI
Maybe this is a dup : Bug 480031 - Context menu for audio files should not have a "View Video" option
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?
Attachment #539790 - Flags: review-
Attachment #539790 - Flags: feedback?(dolske)
Attachment #539790 - Flags: feedback+
> 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.
Attachment #539790 - Attachment is obsolete: true
Attachment #544218 - Flags: review?(dolske)
Oh, I didn't event know that .weba possible, thank you for pointing that out.
I'll update my patch tomorrow.
(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
(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 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.
Attachment #544218 - Flags: review?(dolske) → review+
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.
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.
Status: NEW → ASSIGNED
Component: Video/Audio → Web Services
Component: Web Services → Video/Audio
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
Status: ASSIGNED → NEW
Of course it couldn't work on the first try. Here is the right push :
http://tbpl.mozilla.org/?tree=Try&rev=765d42fb098b
Status: NEW → ASSIGNED
Just figured out this old patch had not been landed. I rebased it and updated the test.
Attachment #544218 - Attachment is obsolete: true
Attachment #615803 - Flags: review?(dolske)
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
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
Whiteboard: [autoland-in-queue] → [autoland-try:-b do -p all -u all]
Whiteboard: [autoland-try:-b do -p all -u all] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
14*"Infrastructure exception"

Trying try again...
Whiteboard: [autoland-try:-b do -p all -u all]
Whiteboard: [autoland-try:-b do -p all -u all]
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
That is, two known orange, and a bunch of interrupted jobs which later completed.
Attachment #615803 - Flags: review?(dolske) → review+
(Nice to see you in video bugs again! :)
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 :-)
Keywords: checkin-needed
Attachment #615803 - Flags: approval-mozilla-central?
Comment on attachment 615803 [details] [diff] [review]
Rebased and updated the test.

[Triage comment]
/browser has a=desktop-only clearance to land.
Attachment #615803 - Flags: approval-mozilla-central?
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b66f3caf2ca
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/1b66f3caf2ca
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: Video/Audio → Menus
Product: Core → Firefox
QA Contact: video.audio → menus
Target Milestone: mozilla14 → ---
Component: Menus → Video/Audio
Product: Firefox → Core
QA Contact: menus → video.audio
Target Milestone: --- → mozilla14
Component: Video/Audio → Menus
Product: Core → Firefox
QA Contact: video.audio → menus
Target Milestone: mozilla14 → Firefox 14
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: