747257 - cant open mp4 videos directly via file:// or http:// when gstreamer support enabled

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[scientes]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120410121533

Steps to reproduce:

passed mp4 file on the command line to fennec
drag and drop mp4 file
"open" mp4 file from download page


Actual results:

file opens in totem (via gnome/xdg-open)


Expected results:

fennec (when built with --enable-gstreamer) should be able to open the file directly


--I was just trying to check out the quality of the default <video> controls, when this got in my way.

Comment 1

[Ralph Giles (:rillian)]

Does it work if you wrap it in an html page with a <video> element?

Loading a raw file is a separate issue from support inside html.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Does Android provide gstreamer libs? AFIK Android uses hardware decoding.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> Does Android provide gstreamer libs? AFIK Android uses hardware decoding.

Scientes is running Fennec on desktop Linux, rather than Android.

Comment 4

[scientes]

h264 video work on numerous websites, including youtube (if you spoof iOS user-agent).

I wasn't able to get a small html file with <video> in it to load the video, but i might just not be well versed in how to use file:// urls

Comment 5

[scientes]

Ok I got it to open via a <video> element in a local file

<video controls>
        <source src="Sintel-Trailer-720p.mp4" type='video/mp4; codecs="avc1.4D401E, mp4a.40.2"'>
</video>

didn't work with <video controls src="Sintel-Trailer-720p.mp4" type='video/mp4; codecs="avc1.4D401E, mp4a.40.2"'/>

Comment 6

[Ralph Giles (:rillian)]

Note: fixing this probably involves adding the mp4 filename extension to the defaultMimeEntries table in uriloader/exthandler/nsExternalHelperAppService.cpp so the correct document wrapper is applied.

(See https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#411)

Comment 7

[scientes]

Attachment: associate with .mp4, .mov, .qt file extentions when --enable-gstreamer

I can't seem to find the VIDEO_OGG, etc macros. Where are they defined? When do I define VIDEO_GSTREAMER?

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 623476 [details] [diff] [review]
associate with .mp4, .mov, .qt file extentions when --enable-gstreamer

Thanks for the patch. A good start.

VIDEO_OGG, etc. are defines for the corresponding mime type, not for the intended playback engine. They're defined in http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h You want to use VIDEO_MPEG or add a new appropriate define for whatever mime types the gstreamer player accepts.

Also, I think we should just provide a mapping for .mp3 and .mp4 files. The mp4 file format is a subset of .mov and .qt, but the second two can have a vary wide variety of codecs in them, not just h.264+aac. Regardless of whether the gstreamer back-end can play any particular combination, we want to limit the formats we support to the bare minimum.

Comment 9

[scientes]

> VIDEO_OGG, etc. are defines for the corresponding mime type, not for the intended playback engine. They're defined in http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h
thanks, I knew they were mime types as a few are in there without using a define

> Also, I think we should just provide a mapping for .mp3 and .mp4 files.
the current bug 422540 patch does not enable mp3 handling with <audio>, so we should wait

>The mp4 file format is a subset of .mov and .qt, but the second two can have a vary wide variety of codecs in them, not just h.264+aac. Regardless of whether the gstreamer back-end can play any particular combination, we want to limit the formats we support to the bare minimum.
done

Comment 10

[scientes]

Attachment: 0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch

I have successfully built firefox in the past, but I am having a problem where gold quits with 

collect2: ld terminated with signal 7 [Bus error]

as I have not been able to resolve this problem I am just uploading now

Comment 11

[scientes]

Comment on attachment 623762 [details] [diff] [review]
0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch

did manage to build fennec, doesn't seem to work, removing feedback?

Comment 12

[scientes]

Attachment: 0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch

I can't figure out why this isn't working
"video/mp4" matches what is in https://bug422540.bugzilla.mozilla.org/attachment.cgi?id=615622
:/

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 623869 [details] [diff] [review]
0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch

Looks good. Does opening an mp4 file directly work with this patch?

Comment 14

[scientes]

> Looks good. Does opening an mp4 file directly work with this patch?

no, and not directly from http either

even with this patch, opening a "Content-Type: video/ogg" video, .ogv, calls

nsDSURIContentListener::DoContent (this=0x7fffc24b9540, 
    aContentType=0x7fffd615f148 "video/ogg", aIsContentPreferred=false, 
    request=0x7fffb437f050, aContentHandler=0x7fffb77fffc8, aAbortProcess=0x7fffffffb56f)

while with "Content-Type: video/mp4", .mp4, calls

nsDSURIContentListener::DoContent(this=0x7fffc2476ec0, 
    aContentType=0x7fffdf5b1228 "application/vnd.mozilla.xul+xml", aIsContentPreferred=false, 
    request=0x7fffb3d599c8, aContentHandler=0x7fffbf9ad2f8, aAbortProcess=0x7fffffffb68f)

----
at docshell/base/nsDSURIContentListener.cpp:135
from nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:709

Comment 15

[scientes]

** the XPCOM registered video/ogg gets called from docshell/base/nsDSURIContentListener.cpp:135

Comment 16

[scientes]

**nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:406

Comment 17

[scientes]

Comment on attachment 623869 [details] [diff] [review]
0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch

>From 9d5c82454b0721777284de7c56674a6a61eca0eb Mon Sep 17 00:00:00 2001
>From: Shawn Landden <[e-mail]>
>Date: Fri, 11 May 2012 02:42:59 -0700
>Subject: [PATCH] Bug 747257 - associate with .mp4 file extentions when
> --enable-gstreamer
>
>---
> netwerk/mime/nsMimeTypes.h                         |    1 +
> .../exthandler/nsExternalHelperAppService.cpp      |    4 ++++
> 2 files changed, 5 insertions(+)
>
>diff --git a/netwerk/mime/nsMimeTypes.h b/netwerk/mime/nsMimeTypes.h
>index c6080d9..925e86f 100644
>--- a/netwerk/mime/nsMimeTypes.h
>+++ b/netwerk/mime/nsMimeTypes.h
>@@ -161,6 +161,7 @@
> #define TEXT_EVENT_STREAM                   "text/event-stream"
> 
> #define VIDEO_MPEG                          "video/mpeg"
>+#define VIDEO_MP4                           "video/mp4"
> #define VIDEO_RAW                           "video/x-raw-yuv"
> #define VIDEO_OGG                           "video/ogg"
> #define VIDEO_WEBM                          "video/webm"
>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp
>index 1e04402..5647492 100644
>--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>@@ -440,6 +440,9 @@ static nsDefaultMimeTypeEntry defaultMimeEntries [] =
>   { VIDEO_WEBM, "webm" },
>   { AUDIO_WEBM, "webm" },
> #endif
>+#ifdef MOZ_GSTREAMER
>+  { VIDEO_MP4, "mp4" },
>+#endif
> #ifdef MOZ_RAW
>   { VIDEO_RAW, "yuv" }
> #endif
>@@ -511,6 +514,7 @@ static nsExtraMimeTypeEntry extraMimeEntries [] =
>   { AUDIO_OGG, "opus", "Opus Audio" },
>   { VIDEO_WEBM, "webm", "Web Media Video" },
>   { AUDIO_WEBM, "webm", "Web Media Audio" },
>+  { VIDEO_MP4, "mp4", "MPEG-4 Multimedia" },
>   { VIDEO_RAW, "yuv", "Raw YUV Video" },
>   { AUDIO_WAV, "wav", "Waveform Audio" }
> };
>-- 
>1.7.9.5
>

Comment 18

[scientes]

Attachment: associate with "video/mp4" streams and .mp4 files when --enable-gstreamer

Works!

Comment 19

[scientes]

Attachment: associate with "video/mp4" streams when --enable-gstreamer

I split the patch into its two logical chunks

Comment 20

[scientes]

Attachment: associate with .mp4 files when --enable-gstreamer

part 2

Comment 21

[scientes]

the second part is the same as the feedback+ patch

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 624971 [details] [diff] [review]
associate with .mp4 files when --enable-gstreamer

r+ if you add an appropriate commit message.

Comment 23

[scientes]

Attachment: use gstreamer for "Content-type: video/mp4" streams when --enable-gstreamer

update commit message

Comment 24

[scientes]

Attachment: Bug 747257 [2/2] - use .mp4 files as "video/mp4" when --enable-gstreamer

update commit message

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 624970 [details] [diff] [review]
associate with "video/mp4" streams when --enable-gstreamer

Works for me! Thanks.

r+ with nits:

Please indent the 'for' block. It's better to overrun 80 columns than to stop indenting, I think.

Like the previous patch, you need a proper commit message, explaining why the change is being made.

It would be nice to not duplicate the mimetype search code now that we have three of these stanzas, but I suppose it's not really worth a helper function. It's acceptable as is.

Comment 26

[Ralph Giles (:rillian)]

Comment on attachment 625255 [details] [diff] [review]
use gstreamer for "Content-type: video/mp4" streams when --enable-gstreamer

Better. How about:

747257 - Load .mp4 files in a document wrapper - r=rillian

When compiled with --enable-gstreamer, use a document loader for video/mp4, and associate that type with the decoder in nsHTMLMediaElement.

Bug 422540 allowed us to use gstreamer to view h.264 videos from within
webpages with <video>, however to view these files when loaded directly we need to recognize their Content-Type.

Comment 27

[Ralph Giles (:rillian)]

Comment on attachment 625256 [details] [diff] [review]
Bug 747257 [2/2] - use .mp4 files as "video/mp4" when --enable-gstreamer

Likewise, this would be better:

Bug 747257 - Add media type mapping for .mp4 - r=rillian

Associate .mp4 files with the "video/mp4" mime type so that we can open them directly via file://

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

For details on how to add a commit message to your patch see https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 29

[scientes]

Attachment: part 1

@kbrosnan I am using git off the github git clone

Comment 30

[scientes]

Attachment: part 2

address issues, I like suggested commit messages

Comment 31

[Ralph Giles (:rillian)]

Comment on attachment 625310 [details] [diff] [review]
part 1

Thanks.

Comment 32

[scientes]

Attachment: Refactor-nsContentUtils-FindInternalConte

I don't really think this is acceptable, because it requires C++11. There is so much context here that a helper function as I can imagine it would be more complex.

I also wrote it with just an array of pointers to the codec lists, (or NULL) and an added loop, but that seemed too reinvent-the-wheel-ish.

I'm use to just:
types = []
types << foo if foo.really?
types << bar if bar.really?
types.each{|type| do_something}

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45118cc6518b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19f0356c09d

Thanks for the patches! For future note, you can just use the checkin-needed flag instead of setting checkin? on the individual patches.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/45118cc6518b
https://hg.mozilla.org/mozilla-central/rev/a19f0356c09d

Comment 35

[Ralph Giles (:rillian)]

Yay. Thanks for the patches, scientes!