The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

Core Graveyard
File Handling
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: scientes, Assigned: scientes)

Tracking

Trunk
mozilla15
x86_64
Linux
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 9 obsolete attachments)

2.26 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.84 KB, patch
rillian
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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"'/>
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 7

5 years ago
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?
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-
(Assignee)

Comment 9

5 years ago
> 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
(Assignee)

Updated

5 years ago
Attachment #623476 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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
Attachment #623762 - Flags: feedback?
(Assignee)

Comment 11

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #623762 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
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
:/
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 14

5 years ago
> 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
(Assignee)

Updated

5 years ago
Summary: cant open mp4 videos via file:// when gstreamer support enabled → cant open mp4 videos via file:// or http:// when gstreamer support enabled
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 15

5 years ago
** the XPCOM registered video/ogg gets called from docshell/base/nsDSURIContentListener.cpp:135
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 16

5 years ago
**nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:406
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

5 years ago
Created attachment 624963 [details] [diff] [review]
associate with "video/mp4" streams and .mp4 files when --enable-gstreamer

Works!
Attachment #624963 - Flags: review?(giles)
(Assignee)

Comment 19

5 years ago
Created attachment 624970 [details] [diff] [review]
associate with "video/mp4" streams when --enable-gstreamer

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)
(Assignee)

Comment 20

5 years ago
Created attachment 624971 [details] [diff] [review]
associate with .mp4 files when --enable-gstreamer

part 2
Attachment #624971 - Flags: review?(giles)
(Assignee)

Comment 21

5 years ago
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+
(Assignee)

Comment 23

5 years ago
Created attachment 625255 [details] [diff] [review]
use gstreamer for "Content-type: video/mp4" streams when --enable-gstreamer

update commit message
Attachment #624970 - Attachment is obsolete: true
Attachment #624970 - Flags: review?(giles)
Attachment #625255 - Flags: review?(giles)
(Assignee)

Comment 24

5 years ago
Created attachment 625256 [details] [diff] [review]
Bug 747257 [2/2] - use .mp4 files as "video/mp4" when --enable-gstreamer

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
(Assignee)

Comment 29

5 years ago
Created attachment 625310 [details] [diff] [review]
part 1

@kbrosnan I am using git off the github git clone
Attachment #625255 - Attachment is obsolete: true
Attachment #625310 - Flags: review?(giles)
(Assignee)

Comment 30

5 years ago
Created attachment 625311 [details] [diff] [review]
part 2

address issues, I like suggested commit messages
Attachment #625256 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
Component: General → File Handling
No longer depends on: 422540
Product: Fennec → Core
QA Contact: general → file-handling
(Assignee)

Updated

5 years ago
Depends on: 422540
(Assignee)

Updated

5 years ago
Attachment #625310 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #625311 - Flags: checkin?
(Assignee)

Comment 32

5 years ago
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}
Attachment #625434 - Flags: feedback?(giles)
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Whiteboard: [checkin-needed] (not the refactor patch)
(Assignee)

Updated

5 years ago
Attachment #625434 - Attachment is obsolete: true
Attachment #625434 - Flags: feedback?(giles)
(Assignee)

Updated

5 years ago
Whiteboard: [checkin-needed] (not the refactor patch) → [checkin-needed]

Updated

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/45118cc6518b
https://hg.mozilla.org/mozilla-central/rev/a19f0356c09d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.