Closed
Bug 557278
Opened 14 years ago
Closed 12 years ago
OGG audio handled as OGV video
Categories
(Firefox :: Menus, defect)
Firefox
Menus
Tracking
()
RESOLVED
FIXED
Firefox 14
People
(Reporter: villalu, Assigned: padenot)
References
()
Details
Attachments
(1 file, 3 obsolete files)
70.42 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•14 years ago
|
||
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
Reporter | ||
Comment 2•13 years ago
|
||
Poke? Still an issue in 6.0a.
Assignee | ||
Updated•13 years ago
|
Status: NEW → UNCONFIRMED
Ever confirmed: false
OS: Mac OS X → All
Assignee | ||
Comment 3•13 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•13 years ago
|
Assignee: nobody → paul
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 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•13 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•13 years ago
|
||
Maybe this is a dup : Bug 480031 - Context menu for audio files should not have a "View Video" option
Comment 12•13 years ago
|
||
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•13 years ago
|
||
> 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•13 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•13 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•13 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
Comment 17•13 years ago
|
||
(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•13 years ago
|
||
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+
Comment 19•13 years ago
|
||
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•13 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
Updated•13 years ago
|
Component: Web Services → Video/Audio
Assignee | ||
Comment 21•13 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•13 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 | ||
Comment 24•12 years ago
|
||
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)
Updated•12 years ago
|
Whiteboard: [autoland-try]
Updated•12 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 25•12 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
Updated•12 years ago
|
Whiteboard: [autoland-in-queue] → [autoland-try:-b do -p all -u all]
Updated•12 years ago
|
Whiteboard: [autoland-try:-b do -p all -u all] → [autoland-in-queue]
Comment 26•12 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•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
Comment 28•12 years ago
|
||
14*"Infrastructure exception" Trying try again...
Whiteboard: [autoland-try:-b do -p all -u all]
Updated•12 years ago
|
Whiteboard: [autoland-try:-b do -p all -u all]
Comment 29•12 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
Comment 30•12 years ago
|
||
That is, two known orange, and a bunch of interrupted jobs which later completed.
Updated•12 years ago
|
Attachment #615803 -
Flags: review?(dolske) → review+
Comment 31•12 years ago
|
||
(Nice to see you in video bugs again! :)
Assignee | ||
Comment 32•12 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•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #615803 -
Flags: approval-mozilla-central?
Comment 33•12 years ago
|
||
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?
Comment 35•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1b66f3caf2ca
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: Video/Audio → Menus
Product: Core → Firefox
QA Contact: video.audio → menus
Target Milestone: mozilla14 → ---
Updated•12 years ago
|
Component: Menus → Video/Audio
Product: Firefox → Core
QA Contact: menus → video.audio
Target Milestone: --- → mozilla14
Updated•12 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.
Description
•