[Browser] For AAC file contained in MP4 container browser showing as "Save as video"

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

()

Core
Audio/Video
P1
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Leo, Assigned: daleharvey)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite +
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [TD-48471] [LeoVB+])

Attachments

(2 attachments, 8 obsolete attachments)

46 bytes, text/x-github-pull-request
benfrancis
: review+
Details | Review | Splinter Review
8.28 KB, patch
daleharvey
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
1. Title: AAC contained in MP4 showing as video
2. Precondition: 
3. Tester's Action: 
                  (1) From FFOS browser open any AAC file contained in MP4.
                  (2) When the embedded player starts playing the content, long press on the player
                  (3) Browser will show the context menu, but it shows "Save Video" instead of "Save audio"

4. Detailed Symptom (ENG.) : Context menu is shown as "Save video" eventhough only audio is there.
6. Reproducibility: Y
1) Frequency Rate : 100%
(Reporter)

Comment 1

5 years ago
The issue happens because when "mozbrowsercontextmenu" event is sent, the content type sent is the type received from the server when we downloaded the content (in this case "video/mp4") , not the actual content type ("audio/..")
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo+
Whiteboard: [TD-48471]

Comment 2

5 years ago
Paul Adenot worked on a similar issue with WebM and Ogg in Bug 557278. CC'ing him to see if he has any insight as to whether a similar fix would work.
Just put something like that somewhere:

> video.readyState >= this.target.HAVE_METADATA &&
>  (video.videoWidth == 0 || video.videoHeight == 0)

that is, make sure we have metadata (so we actually know the video dimensions, if any), and if the video dimension are zero, you are dealing with an audio only file. If this expression is true, you have an audio-only file, otherwise, a video track is present.

This is exactly what I did in bug 557278.

In the future, we will have AudioTrack and VideoTrack support for media elements, so we will be able to remove this little hack, but for now, this is the way to go.
After investigation, this bug may need the help of gecko. When gaia receiving mozbrowsercontextmenu event, it only has nodeName without readyState, videoWidth, and videoHeight, like:

{"systemTargets":[{"nodeName":"VIDEO","data":"http://download.wavetlan.com/SVV/Media/HTTP/AAC/Media-Convert/Media-Convert_test1_AAC-LC_v4_Stereo_VBR_64kbps_44100Hz.mp4"}],"contextmenu":null,"msg_name":"contextmenu"}

So, it is not possible to do it under gaia, this bug needs the help of gecko to pass more information to gaia, or do the probing for gaia.

Comment 5

5 years ago
(In reply to John Hu [:johnhu] from comment #4)
> So, it is not possible to do it under gaia, this bug needs the help of gecko
> to pass more information to gaia, or do the probing for gaia.

Can you use DOM functions to access the element given the node name and get the attributes from there?
I don't think so. If I make the query to this iframe, its contentWindow and contentDocument are null. It's the cross domain issue.

Comment 7

5 years ago
Over to roc to reassign this leo+ bug. Please let us know if there's any chance this will land before 6/24, if not we should change the milestone.
Assignee: nobody → roc
Hi Wayne, 
This has QE3 target milestone with [TD] on white board. So, I think We should make this TaiwanWW.
Hi Wayne, Please ignore the previous comment. 
I made a mistake.
Thank you.
(Assignee)

Comment 10

5 years ago
Yeh we cant access the dom element since its cross frame. We can either add data to the context menu api (its already a fairly adhoc api) or create a new video element based on the context menu data and test is again.

I would prefer not to add to the API, but at the same time I really dont like the idea of creating duplicated video etc elements inside the browser since this would be inside the system process.

Given the above (creating media elements inside the system process sounds very scary, I will look at adding the necessary data to the api)
Yes, this needs to be added to the context menu API. Should be easy enough to add to BrowserElementChild.js. I suggest you add a "hasVideo" field to any video elements in systemTargets and do what Paul suggested in comment #3.
Assignee: roc → dale
(Assignee)

Comment 12

5 years ago
isnt the problem here that we are launching an embedded video player for audio content? desktop doesnt have this problem and shows an 'embed' when navigating a link to audio files, seems like <audio would be preferable but either way not video

Related is we are showing fullscreen controls in the embedded media player (https://bugzilla.mozilla.org/show_bug.cgi?id=885974)

Updated

5 years ago
Blocks: 885974
(In reply to Dale Harvey (:daleharvey) from comment #12)
> isnt the problem here that we are launching an embedded video player for
> audio content? desktop doesnt have this problem and shows an 'embed' when
> navigating a link to audio files, seems like <audio would be preferable but
> either way not video

<embed>? It sounds like your desktop build doesn't support MP4 so it's being delegated to a plugin instead. Try in a profile where you've disabled all plugins. Better still, try in a Windows or Linux GStreamer desktop build where MP4/AAC support is enabled.
(Assignee)

Comment 14

5 years ago
Yeh if I use ogg audio then I get the proper embedded audio page, in which this bug wouldnt happen.

The problem is inside the logic of where we decide to launch the videoDocument vs audio page vs download unrecognised files, it can be seen when accessing audio files which we do not support as well
(Assignee)

Comment 15

5 years ago
and just so I dont lose it again, on desktop it runs though http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#6009 but not sure about b2g yet
(Assignee)

Comment 16

4 years ago
Created attachment 768350 [details] [diff] [review]
Dont create VideoDocument for unsupported media
Attachment #768350 - Flags: review?(chris.double)
(Assignee)

Comment 17

4 years ago
Comment on attachment 768350 [details] [diff] [review]
Dont create VideoDocument for unsupported media

Clearing review, this is on the wrong bug, apologies
Attachment #768350 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
No longer blocks: 885974
(Assignee)

Comment 18

4 years ago
As mentioned in 885974 I went wrong debugging this, we do need to implement the hasVideo property on the contextmenu data, that patch shouldnt take long
(Assignee)

Comment 19

4 years ago
Created attachment 768699 [details] [diff] [review]
dd flag to context menu to detect audio being played in video elements.
Attachment #768699 - Flags: review?(justin.lebar+bug)
Comment on attachment 768699 [details] [diff] [review]
dd flag to context menu to detect audio being played in video elements.

r=me, but this really needs a test, and I'd be surprised if this doesn't break our existing test.  (If it doesn't break the existing test, it's not testing things particularly well.  :)

We'll also need to fix up Gaia to handle the new API, of course.

I'll leave it up to release drivers whether to require an automated test before landing.  They've been the ones beating the quality/automated tests drum lately.
Attachment #768699 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 21

4 years ago
Created attachment 768713 [details]
dd flag to context menu to detect audio being played in video elements.
(Assignee)

Updated

4 years ago
Attachment #768713 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 768716 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Added tests, carrying r+, will do a try push before landing
Attachment #768699 - Attachment is obsolete: true
Attachment #768716 - Flags: review+
(Assignee)

Comment 23

4 years ago
Created attachment 768723 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Adding actual new patch with tests, carrying r+ 

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6a67a387193b
Attachment #768350 - Attachment is obsolete: true
Attachment #768716 - Attachment is obsolete: true
Attachment #768723 - Flags: review+
(Assignee)

Comment 24

4 years ago
Created attachment 768776 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Try (https://tbpl.mozilla.org/?tree=Try&rev=6a67a387193b) is failing android, seems likely that android is avoiding loading metadata so pushed a new try with preload="metadata" https://tbpl.mozilla.org/?tree=Try&rev=9d55dd74abbc
Attachment #768723 - Attachment is obsolete: true
Attachment #768776 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 768780 [details] [review]
Gaia patch for hasVideo context menu fix
Attachment #768780 - Flags: review?(bfrancis)
Comment on attachment 768780 [details] [review]
Gaia patch for hasVideo context menu fix

Seems weird to have to ask whether a video has video, but I'm not reviewing the Gecko part so r+me
Attachment #768780 - Flags: review?(bfrancis) → review+
Gaia part landed on master, I'm guessing I just regressed that feature until the Gecko part lands, only just noticed it didn't land yet.

https://github.com/mozilla-b2g/gaia/commit/70f9a1761990d57be284e305554734ffb4eb4eef

Will need uplift to v1-train, should uplift Gecko part at the same time.
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected

Updated

4 years ago
Flags: in-moztrap?(cschmoeckel)
(Assignee)

Comment 28

4 years ago
Apologies, I should have flagged dont check in, did some more work on the tests to get android to hopefully play nice, will need a new review at this point but will wait until try is passing

https://tbpl.mozilla.org/?tree=Try&rev=a5aed799628b
(Assignee)

Comment 29

4 years ago
Created attachment 770010 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Rerequesting review on this since most of the code change is the newly introduced tests and the android tests have just started passing try

https://tbpl.mozilla.org/?tree=Try&rev=a5aed799628b
Attachment #768776 - Attachment is obsolete: true
Attachment #770010 - Flags: review?(justin.lebar+bug)

Comment 30

4 years ago
This bug has existing (but new) Browser Test Cases for both audio and video files. Added additional steps to these TCs to have a weblink with files available for downloading. 
TC #6890 - [Browser] User can save a video using a gesture
TC #6891 - [Browser] User can save an audio using a gesture
Flags: in-moztrap?(cschmoeckel) → in-moztrap+
Comment on attachment 770010 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

# HG changeset patch
# User Dale Harvey <dale@arandomurl.com>
Bug 885349 - Add flag to context menu to detect audio being played in video elements. r=jlebar

>diff --git a/content/media/test/audio.ogg b/content/media/test/audio.ogg
>new file mode 100644

Does this file need to be so big?  Could you just use priority/silence.ogg?
You even could move it up a directory and fix the few tests that already use
it, if you were feeling brave...  :)

Did you forget to git add example.ogv?  I don't see where it comes from, and I
don't see a file with that name in my tree.  But the same question applies: Can
we reuse an existing ogv file from the tree?  (git ls-files '*.ogv' has a
bunch.)

I ask because we don't want to add new, large files to the tree if we can help
it.  That just slows things down for everyone.

diff --git a/dom/browser-element/mochitest/Makefile.in b/dom/browser-element/mochitest/Makefile.in
index 3545291..ab74d8c 100644
--- a/dom/browser-element/mochitest/Makefile.in
+++ b/dom/browser-element/mochitest/Makefile.in
@@ -11,16 +11,17 @@ relativesrcdir  = @relativesrcdir@

> MOCHITEST_FILES = \
>+                $(topsrcdir)/content/media/test/audio.ogg \
> 		file_empty_script.js \
> 		file_empty.html \
> 		file_focus.html \
> 		browserElementTestHelpers.js \
> 		test_browserElement_NoAttr.html \
> 		test_browserElement_NoPref.html \
> 		test_browserElement_NoWhitelist.html \
> 		browserElement_LoadEvents.js \

This looks OK, but I'm not positive this is the right way to include a
cross-tree file.  Can you get this signed off by a build peer, maybe on IRC or
something?

Or maybe you'll end up using slience.ogg and this won't be an issue.  :)

diff --git a/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js b/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
index 68041e7..c3f3f15 100644
--- a/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
+++ b/dom/browser-element/mochitest/browserElement_ContextmenuEvents.js

Can we add a check for audio in <audio> while we're at it?
 
>+function sendSrcTo(selector, src, callback) {
>+  mm.sendAsyncMessage('setsrc', { 'selector': selector, 'src': src });
>+  mm.addMessageListener('test:srcset', function onSrcSet(msg) {
>+    mm.removeMessageListener('test:srcset', onSrcSet);
>+    callback();
>+  });
>+}

If I understand what you're trying to do, I think an alternative way to do this
would be to set the src in the <video> tag itself, and then have the test page
send a message once the metadata has loaded.  Only once we received that
message would we start the test.

Anyway, this is fine, but that idiom may be cleaner for future tests.

r=me with the questions about the media files cleared up.
Attachment #770010 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 32

4 years ago
> Does this file need to be so big?  Could you just use priority/silence.ogg?

Adding audio.ogg that way was a mistake, it was used in other tests and I thought it was best to copy it over to keep the tests isolated, then used it in another test and forgot about the original. I will copy across the original and verify thats the right way to cross reference. I have seen other tests do the same though.

> Did you forget to git add example.ogv?  

I didnt strictly need that to reference a valid file and didnt want to add a new one, will copy along an existing one (seems more sane to have it point to a video)

> Can we add a check for audio in <audio> while we're at it?

Will do

> If I understand what you're trying to do, I think an alternative way to do this
> would be to set the src in the <video> tag itself, and then have the test page 
> send a message once the metadata has loaded.

The metadataloaded event wont be triggered if we add the eventListener once its loaded, this way seemed more robust than doing an |if (audio.HAVE_METADATA) { sendMessage } else { audio.addEventListener.. }| Since we would also need to have |checkAudioinVideoContextMenu()| wait for the message if it hadnt been sent already as well, this way seemed cleaner and easier to follow.
When you say "copy across the original", do you mean to copy it via the makefile, or to make a copy in the source tree?  The former seems preferable to me.
(Assignee)

Comment 34

4 years ago
> When you say "copy across the original", do you mean to copy it via the makefile, or to make a copy in the source tree?  The former seems preferable to me.

Yup copy it in the makefile
(Assignee)

Comment 35

4 years ago
Created attachment 771115 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Updated based on review, carrying the review, will merge in the morning
Attachment #770010 - Attachment is obsolete: true
Attachment #771115 - Flags: review+
(Assignee)

Comment 36

4 years ago
I am having problems with my L3 access so flagging checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/9b2114c04762
Flags: in-testsuite+
Keywords: checkin-needed
Backed out for mochitest timeouts.
https://hg.mozilla.org/projects/birch/rev/1afda0c66c23

https://tbpl.mozilla.org/php/getParsedLog.php?id=25066619&tree=Birch
(Assignee)

Comment 39

4 years ago
hmm, something has changed in the mochitest format, failing here too (thankfully)

I am getting video / mimetype not supported for audio.ogg served as application/ogg, seems like it should work, checking out
(Assignee)

Comment 40

4 years ago
Created attachment 772802 [details] [diff] [review]
add flag to context menu to detect audio being played in video elements.

Something since the last try push changed data uri's to have their own origin which made the relative urls break, have switched them to absolute urls, repushed this patch to try and will mark checkin-needed once it passes, carrying review as it was a very minor change

https://tbpl.mozilla.org/?tree=Try&rev=f88acb245e7c
Attachment #771115 - Attachment is obsolete: true
Attachment #772802 - Flags: review+
(Assignee)

Comment 41

4 years ago
Try is passing again, marking checkin-needed, apologies for the churn
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/35b639eb4c39
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35b639eb4c39
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f6a12e4395db
status-b2g18: affected → fixed
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f6a12e4395db
status-b2g-v1.1hd: affected → fixed

Updated

4 years ago
Whiteboard: [TD-48471] → [TD-48471] [LeoVB+]
(Assignee)

Comment 46

4 years ago
Uplifted gaia side to v1-train, next time there is seperate gaia + gecko patches I will file a seperate bug

https://github.com/mozilla-b2g/gaia/commit/b035ede5863ac0db0493a3c47e44213101f2a775
Duplicate of this bug: 900396
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.