Closed Bug 747257 Opened 9 years ago Closed 9 years ago

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

Categories

(Core Graveyard :: File Handling, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: shawnlandden, Assigned: shawnlandden)

References

()

Details

Attachments

(2 files, 9 obsolete files)

2.26 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.84 KB, patch
rillian
: review+
Details | Diff | Splinter Review
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.
Depends on: 422540
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.
Does Android provide gstreamer libs? AFIK Android uses hardware decoding.
(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.
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
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"'/>
Summary: cant open mp4 videos via file:// when gstreamer support enabled (wontfix?) → cant open mp4 videos via file:// when gstreamer support enabled
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)
I can't seem to find the VIDEO_OGG, etc macros. Where are they defined? When do I define VIDEO_GSTREAMER?
Attachment #623476 - Flags: feedback?(giles)
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.
Attachment #623476 - Flags: feedback?(giles) → feedback-
> 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
Attachment #623476 - Attachment is obsolete: true
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
Attachment #623762 - Flags: feedback?
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?
Attachment #623762 - Flags: feedback?
Attachment #623762 - Attachment is obsolete: true
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
:/
Attachment #623869 - Flags: feedback?(giles)
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?
Attachment #623869 - Flags: feedback?(giles) → feedback+
> 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
Summary: cant open mp4 videos via file:// when gstreamer support enabled → cant open mp4 videos via file:// or http:// when gstreamer support enabled
Summary: cant open mp4 videos via file:// or http:// when gstreamer support enabled → cant open mp4 videos via file:// or http:// (direct) when gstreamer support enabled
** the XPCOM registered video/ogg gets called from docshell/base/nsDSURIContentListener.cpp:135
Summary: cant open mp4 videos via file:// or http:// (direct) when gstreamer support enabled → cant open mp4 videos directly via file:// or http:// when gstreamer support enabled
**nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:406
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
>
Attachment #623869 - Attachment is obsolete: true
Works!
Attachment #624963 - Flags: review?(giles)
I split the patch into its two logical chunks
Attachment #624963 - Attachment is obsolete: true
Attachment #624963 - Flags: review?(giles)
Attachment #624970 - Flags: review?(giles)
part 2
Attachment #624971 - Flags: review?(giles)
the second part is the same as the feedback+ patch
Assignee: nobody → instigatorirc
Comment on attachment 624971 [details] [diff] [review]
associate with .mp4 files when --enable-gstreamer

r+ if you add an appropriate commit message.
Attachment #624971 - Flags: review?(giles) → review+
update commit message
Attachment #624970 - Attachment is obsolete: true
Attachment #624970 - Flags: review?(giles)
Attachment #625255 - Flags: review?(giles)
update commit message
Attachment #624971 - Attachment is obsolete: true
Attachment #625256 - Flags: review?(giles)
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.
Attachment #624970 - Flags: review+
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.
Attachment #625255 - Flags: review?(giles) → review-
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://
Attachment #625256 - Flags: review?(giles) → review-
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch part 1Splinter Review
@kbrosnan I am using git off the github git clone
Attachment #625255 - Attachment is obsolete: true
Attachment #625310 - Flags: review?(giles)
Attached patch part 2Splinter Review
address issues, I like suggested commit messages
Attachment #625256 - Attachment is obsolete: true
Attachment #625311 - Flags: review?(giles)
Comment on attachment 625310 [details] [diff] [review]
part 1

Thanks.
Attachment #625310 - Flags: review?(giles) → review+
Attachment #625311 - Flags: review?(giles) → review+
Component: General → File Handling
No longer depends on: 422540
Product: Fennec → Core
QA Contact: general → file-handling
Depends on: 422540
Attachment #625310 - Flags: checkin?
Attachment #625311 - Flags: checkin?
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}
Attachment #625434 - Flags: feedback?(giles)
Target Milestone: --- → mozilla15
Whiteboard: [checkin-needed] (not the refactor patch)
Attachment #625434 - Attachment is obsolete: true
Attachment #625434 - Flags: feedback?(giles)
Whiteboard: [checkin-needed] (not the refactor patch) → [checkin-needed]
Keywords: checkin-needed
Whiteboard: [checkin-needed]
Attachment #625310 - Flags: checkin?
Attachment #625311 - Flags: checkin?
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.
Flags: in-testsuite-
Keywords: checkin-needed
Yay. Thanks for the patches, scientes!
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.