OGG audio handled as OGV video

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Menus
--
major
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Luis Villa [Outside Counsel; for non-law use luis@tieguy.org], Assigned: padenot)

Tracking

Trunk
Firefox 14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Updated

6 years ago
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Mac OS X → All
(Assignee)

Comment 3

6 years ago
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)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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).
Attachment #539606 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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)

Comment 9

6 years ago
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

6 years ago
Maybe this is a dup : Bug 480031 - Context menu for audio files should not have a "View Video" option
(Assignee)

Updated

6 years ago
Duplicate of this bug: 480031
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+
(Assignee)

Comment 13

6 years ago
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.
Attachment #539790 - Attachment is obsolete: true
Attachment #544218 - Flags: review?(dolske)

Comment 14

6 years ago
(In reply to comment #13)
> WebM cannot (as far as I know) contains only audio, so the MIME type check
> works, without having to rely on other checks.
> 

FWIW, some related discussions on webm-discuss@webmproject.org :
http://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/e76771b04faf60c9/
http://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/93d0575d526ea5fc
http://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/6217a9e16cbc52ff/
(Assignee)

Comment 15

6 years ago
Oh, I didn't event know that .weba possible, thank you for pointing that out.
I'll update my patch tomorrow.

Comment 16

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
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
(Assignee)

Comment 21

6 years ago
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
(Assignee)

Comment 22

6 years ago
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
(Assignee)

Updated

6 years ago
Duplicate of this bug: 497666
(Assignee)

Comment 24

5 years ago
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.
Attachment #544218 - Attachment is obsolete: true
Attachment #615803 - Flags: review?(dolske)
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 25

5 years ago
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]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all] → [autoland-in-queue]

Comment 26

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
14*"Infrastructure exception"

Trying try again...
Whiteboard: [autoland-try:-b do -p all -u all]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all]

Comment 29

5 years ago
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! :)
(Assignee)

Comment 32

5 years ago
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 :-)
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
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

Updated

5 years ago
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.