Last Comment Bug 747257 - cant open mp4 videos directly via file:// or http:// when gstreamer support enabled
: cant open mp4 videos directly via file:// or http:// when gstreamer support e...
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: scientes
:
Mentors:
http://m.democracynow.org/
Depends on: 422540
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-19 17:10 PDT by scientes
Modified: 2016-06-22 12:16 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
associate with .mp4, .mov, .qt file extentions when --enable-gstreamer (1.40 KB, patch)
2012-05-12 22:35 PDT, scientes
giles: feedback-
Details | Diff | Review
0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch (1.18 KB, patch)
2012-05-14 12:23 PDT, scientes
no flags Details | Diff | Review
0001-Bug-747257-associate-with-.mp4-file-extentions-when-.patch (1.77 KB, patch)
2012-05-14 16:51 PDT, scientes
giles: feedback+
Details | Diff | Review
associate with "video/mp4" streams and .mp4 files when --enable-gstreamer (3.40 KB, patch)
2012-05-17 17:11 PDT, scientes
no flags Details | Diff | Review
associate with "video/mp4" streams when --enable-gstreamer (1.92 KB, patch)
2012-05-17 17:22 PDT, scientes
giles: review+
Details | Diff | Review
associate with .mp4 files when --enable-gstreamer (1.76 KB, patch)
2012-05-17 17:22 PDT, scientes
giles: review+
Details | Diff | Review
use gstreamer for "Content-type: video/mp4" streams when --enable-gstreamer (2.15 KB, patch)
2012-05-18 14:04 PDT, scientes
giles: review-
Details | Diff | Review
Bug 747257 [2/2] - use .mp4 files as "video/mp4" when --enable-gstreamer (1.95 KB, patch)
2012-05-18 14:04 PDT, scientes
giles: review-
Details | Diff | Review
part 1 (2.26 KB, patch)
2012-05-18 17:19 PDT, scientes
giles: review+
Details | Diff | Review
part 2 (1.84 KB, patch)
2012-05-18 17:20 PDT, scientes
giles: review+
Details | Diff | Review
Refactor-nsContentUtils-FindInternalConte (3.58 KB, patch)
2012-05-19 12:39 PDT, scientes
no flags Details | Diff | Review

Description scientes 2012-04-19 17:10:55 PDT
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) needinfo me 2012-04-19 17:13:31 PDT
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 [:kbrosnan] 2012-04-19 17:15:10 PDT
Does Android provide gstreamer libs? AFIK Android uses hardware decoding.
Comment 3 Matt Brubeck (:mbrubeck) 2012-04-19 17:22:25 PDT
(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 2012-04-19 17:31:57 PDT
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 2012-04-19 17:57:55 PDT
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) needinfo me 2012-05-09 10:38:06 PDT
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 2012-05-12 22:35:35 PDT
Created attachment 623476 [details] [diff] [review]
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) needinfo me 2012-05-13 19:29:15 PDT
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 2012-05-13 21:56:43 PDT
> 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 2012-05-14 12:23:59 PDT
Created attachment 623762 [details] [diff] [review]
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 2012-05-14 13:46:28 PDT
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 2012-05-14 16:51:10 PDT
Created attachment 623869 [details] [diff] [review]
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) needinfo me 2012-05-15 16:13:48 PDT
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 2012-05-17 13:48:14 PDT
> 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 2012-05-17 15:39:36 PDT
** the XPCOM registered video/ogg gets called from docshell/base/nsDSURIContentListener.cpp:135
Comment 16 scientes 2012-05-17 15:59:35 PDT
**nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:406
Comment 17 scientes 2012-05-17 17:08:20 PDT
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 <shawnlandden@gmail.com>
>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 2012-05-17 17:11:55 PDT
Created attachment 624963 [details] [diff] [review]
associate with "video/mp4" streams and .mp4 files when --enable-gstreamer

Works!
Comment 19 scientes 2012-05-17 17:22:17 PDT
Created attachment 624970 [details] [diff] [review]
associate with "video/mp4" streams when --enable-gstreamer

I split the patch into its two logical chunks
Comment 20 scientes 2012-05-17 17:22:57 PDT
Created attachment 624971 [details] [diff] [review]
associate with .mp4 files when --enable-gstreamer

part 2
Comment 21 scientes 2012-05-17 18:18:54 PDT
the second part is the same as the feedback+ patch
Comment 22 Ralph Giles (:rillian) needinfo me 2012-05-18 13:25:10 PDT
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 2012-05-18 14:04:04 PDT
Created attachment 625255 [details] [diff] [review]
use gstreamer for "Content-type: video/mp4" streams when --enable-gstreamer

update commit message
Comment 24 scientes 2012-05-18 14:04:54 PDT
Created attachment 625256 [details] [diff] [review]
Bug 747257 [2/2] - use .mp4 files as "video/mp4" when --enable-gstreamer

update commit message
Comment 25 Ralph Giles (:rillian) needinfo me 2012-05-18 15:25:58 PDT
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) needinfo me 2012-05-18 15:45:09 PDT
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) needinfo me 2012-05-18 15:47:23 PDT
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 [:kbrosnan] 2012-05-18 16:16:31 PDT
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 2012-05-18 17:19:45 PDT
Created attachment 625310 [details] [diff] [review]
part 1

@kbrosnan I am using git off the github git clone
Comment 30 scientes 2012-05-18 17:20:38 PDT
Created attachment 625311 [details] [diff] [review]
part 2

address issues, I like suggested commit messages
Comment 31 Ralph Giles (:rillian) needinfo me 2012-05-19 09:43:53 PDT
Comment on attachment 625310 [details] [diff] [review]
part 1

Thanks.
Comment 32 scientes 2012-05-19 12:39:32 PDT
Created attachment 625434 [details] [diff] [review]
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] 2012-05-20 16:14:05 PDT
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 35 Ralph Giles (:rillian) needinfo me 2012-05-21 08:55:56 PDT
Yay. Thanks for the patches, scientes!

Note You need to log in before you can comment on or make changes to this bug.