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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Leo]

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%

Comment 1

[Leo]

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/..")

Comment 2

[cajbir (:cajbir)]

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.

Comment 3

[Paul Adenot (:padenot)]

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.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

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

[cajbir (:cajbir)]

(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?

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

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

[Alex Keybl [:akeybl]]

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.

Comment 8

[Jinho Hwang [:jeffhwang]]

Hi Wayne, 
This has QE3 target milestone with [TD] on white board. So, I think We should make this TaiwanWW.

Comment 9

[Jinho Hwang [:jeffhwang]]

Hi Wayne, Please ignore the previous comment. 
I made a mistake.
Thank you.

Comment 10

[Dale Harvey (:daleharvey)]

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)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Dale Harvey (:daleharvey)]

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)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Dale Harvey (:daleharvey)]

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

Comment 15

[Dale Harvey (:daleharvey)]

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

Comment 16

[Dale Harvey (:daleharvey)]

Attachment: Dont create VideoDocument for unsupported media

Comment 17

[Dale Harvey (:daleharvey)]

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

Clearing review, this is on the wrong bug, apologies

Comment 18

[Dale Harvey (:daleharvey)]

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

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: dd flag to context menu to detect audio being played in video elements.

Comment 20

[Justin Lebar (not reading bugmail)]

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.

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: dd flag to context menu to detect audio being played in video elements.

Comment 22

[Dale Harvey (:daleharvey)]

Attachment: add flag to context menu to detect audio being played in video elements.

Added tests, carrying r+, will do a try push before landing

Comment 23

[Dale Harvey (:daleharvey)]

Attachment: 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

Comment 24

[Dale Harvey (:daleharvey)]

Attachment: 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

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: Gaia patch for hasVideo context menu fix

Comment 26

[Ben Francis [:benfrancis]]

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

Comment 27

[Ben Francis [:benfrancis]]

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.

Comment 28

[Dale Harvey (:daleharvey)]

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

Comment 29

[Dale Harvey (:daleharvey)]

Attachment: 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

Comment 30

[:CSchmoeckel]

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

Comment 31

[Justin Lebar (not reading bugmail)]

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 <[e-mail]>
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.

Comment 32

[Dale Harvey (:daleharvey)]

> 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.

Comment 33

[Justin Lebar (not reading bugmail)]

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.

Comment 34

[Dale Harvey (:daleharvey)]

> 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

Comment 35

[Dale Harvey (:daleharvey)]

Attachment: 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

Comment 36

[Dale Harvey (:daleharvey)]

I am having problems with my L3 access so flagging checkin-needed

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/9b2114c04762

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest timeouts.
https://hg.mozilla.org/projects/birch/rev/1afda0c66c23

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

Comment 39

[Dale Harvey (:daleharvey)]

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

Comment 40

[Dale Harvey (:daleharvey)]

Attachment: 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

Comment 41

[Dale Harvey (:daleharvey)]

Try is passing again, marking checkin-needed, apologies for the churn

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/35b639eb4c39

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35b639eb4c39

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f6a12e4395db

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f6a12e4395db

Comment 46

[Dale Harvey (:daleharvey)]

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