We should be able to load an Ogg link directly the same way we can display an image directly. We might even go further and register ourselves with the OS as a handler for Ogg file types. We should probably do something like nsImageDocument, and create an anonymous video element with sensible defaults.

(In reply to comment #0) > We should be able to load an Ogg link directly the same way we can display an > image directly. We might even go further and register ourselves with the OS as > a handler for Ogg file types. Please make sure if you do that, that it is non-destructive to the previous settings for Ogg file types. Is there any intention for something like about:ogg for Gecko - A small bit of evangelism and a link to Xiph’s platform-native codecs?

Is "Layout" correct Component for this? or is it a "Video/Audio" ?

Getting this to work correctly is a bit hard at the moment. Gecko starts loading the file, gets the content type, then nsContentDLF figures out what kind of document to create. We can then create an nsVideoDocument with the right content in it, *but* instead of having that video element initiate a new load with the same URL, we need to pass the already-open channel to the video element and have it use that. This is what plugin and image documents already do. The thing is, right now the backends are responsible for opening the channels. We need to factor that out and up so the video element takes care of that; then it can also adopt the channel passed to it by the nsVideoDocument.

The backends open the channel at the moment because they open them in different ways depending on the channel type. A file channel gets opened synchronously and uses the seek/tell methods to do fast seeking and not having to buffer, etc. Other channel types are opened asynchronously and have a listener attached that feeds a pipe that is read on another thread. The original channel can be closed and reopened to do HTTP range requests for seeking, etc. If we're passed an already opened channel, is it opened asynchronously and we get the data from a listener? Or is the channel opened synchronously? How do we get the data out of an already opened channel?

It's opened asynchronously. nsContentDLF::CreateInstance gets to pass back a listener which is attached to the stream, and whichever document is created gets to pass back that listener; the existing plugin/image documents set up and return a listener which proxies OnStartRequest/OnDataAvailable etc to a listener provided by the plugin or image-loader. So we'll want an interface on the video element which does something similar --- sets it up with a channel and returns the video element's stream listener to be attached. We won't get the benefit of doing synchronous file access. We should still be able to seek properly and not buffer if it's a file stream, though, right?

I suppose that trying to reuse the same channel isn't going to work when we do seeking, because there we have to open new channels for different ranges. Sigh. Still, I think we should make nsVideoDecoder::Load take a channel and return a listener, and try to have the decoders use that channel as much as possible. When they really need to open a new channel for the URI, they can do that.

Yes, I agree. I'll change the interface to do that. New bug or should I make the change and attach a patch for that to this one?

the latter's fine.

(In reply to comment #2) I sort of agree with comment #2 that it should be possible to go back to previous settings. Somehow my Tools > Options > Applications for application/ogg and x-ogg was set to "Use QuickTime Plug-in 7.4 (in Minefield)", probably because I installed some wacky codec pack, and I might want to switch ogg content handling between that plug-in, an external app, and direct loading in Firefox. I think the UI answer is to have an explicit "Preview in Firefox" option in Tools > Options > Applications , even for built-in mime types. I have such a choice for Podcast, Video Podcast, and Web feed, but not for other mime types. I realize that makes the fix more complicated, but it would address some other bugs about handling files within Firefox/in a plug-in/in an external app. BTW, I tried to get FF 3.1 to load ogg files by changing them in Tools > Options > Applications to "Use Firefox". 200 tabs later, bad idea :-) (bug 218257).

This seems to work now with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre. Marking as fixed by bug 422538.

Are you sure it's not a plugin handling the video? It doesn't work on OS X, and I don't think and core support for doing this landed. EG, loading the URL on this bug just gives me a Save File dialog (server is sending it as application/ogg, if that matters).

This is totally not fixed.

This patch seems to work pretty well. I can play local Ogg files and .ogg links on the Web. -- Create nsVideoDocument, which is like nsImageDocument and nsPluginDocument but simpler than either because I'm not providing any special UI nor do I need any complex instantiation. We just have a <video> element with autoplay and controls set. -- Add LoadWithChannel API to nsHTMLMediaElement so we can set up the element with an existing channel; this bypasses the normal src/source picking, although I try to share as much code as possible. -- Add static methods to register the media types we can handle, and to check whether we handle a particular type. -- From nsMediaDecoder down to nsMediaStream and the stream-strategies, push nsIChannel and nsIStreamListener** parameters so that we can read from an existing channel. Since we don't get to open the channel and pass in our stream-listener, we have to return it instead via the nsIStreamListener**; it gets passed out to nsVideoDocument where it gets hooked up. -- Note that seeking, or replaying after the first time through, requires us to reopen the URI. Too bad. -- Hook up nsContentDLF to create video documents for known media MIME types. Note that we create video documents for audio MIME types. This is OK; if there's no video track, the <video> element should still do something sensible. (Currently it might not, but that's a separate bug.) -- Add MIME type mappings for the "oga", "ogv" and "ogg" file extensions. These are hardcoded and cannot be overridden by the platform. Perhaps they should be in extraMimeEntries instead? I'm not sure. Chris, I just want your review on the content/media bits.

BTW, it would slightly simplify the code if we made nsMediaDecoder and nsMediaStream and related interfaces *always* take an nsIChannel instead of an nsIURI, so the opening of the channel would move out to nsHTMLMediaElement. The only problem I can see with that is that for file URIs loaded normally, we would open an async channel which we don't really want (since we either want a sync channel, like we have in the current code, or to open the file directly, like I have in this patch); currently we avoid opening such an unnecessary channel.

Comment on attachment 345065 [details] [diff] [review] fix >+++ b/content/html/content/public/nsHTMLMediaElement.h >+ PRBool CreateDecoder(const nsACString& aType); Document what the return value means? And maybe aMIMEType? Also document the other new functions here? >+++ b/content/html/content/src/nsHTMLMediaElement.cpp > NS_IMETHODIMP nsHTMLMediaElement::Load() > { >+ return LoadWithChannel(nsnull, nsnull); >+} >+ >+nsresult nsHTMLMediaElement::LoadWithChannel(nsIChannel *aChannel, >+ nsIStreamListener **aListener) >+{ >+ *aListener = nsnull; Won't that crash if someone calls load()? Please add a test for this! >+PRBool nsHTMLMediaElement::CanHandleMediaType(const char* aMIMEType) >+{ >+#if MOZ_OGG #ifdef? >+void nsHTMLMediaElement::InitMediaTypes() >+ catMan->AddCategoryEntry("Gecko-Content-Viewers", gOggTypes[i], >+ "@mozilla.org/content/document-loader-factory;1", >+ PR_TRUE, PR_TRUE, nsnull); Would it make sense to pass PR_FALSE for aPersist like SVG does? >+nsresult nsHTMLMediaElement::PickMediaElement() > if (HasAttr(kNameSpaceID_None, nsGkAtoms::src)) { > if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) { Why do we do that HasAttr check? > const nsAFlatCString &charset = doc->GetDocumentCharacterSet(); nsContentUtils::NewURIWithDocumentCharset? >+++ b/content/html/document/src/nsVideoDocument.cpp >+ * Portions created by the Initial Developer are Copyright (C) 1998 2008? And fix the name of the original developer? >+nsVideoDocument::StartDocumentLoad(const char* aCommand, >+ element->SetAutoplay(PR_TRUE); >+ element->SetControls(PR_TRUE); >+ element->LoadWithChannel(aChannel, aListener); >+ >+ return body->AppendChildTo(element, PR_FALSE); Do you need to tell the element the src somewhere here? Otherwise, how will it know what it needs to reload if it needs to reload? Or how will context menus, etc, know the right URI? >+++ b/content/media/video/public/nsMediaDecoder.h >+ // Start downloading the video. Decode the downloaded data up to the >+ // point of the first frame of data. >+ // Exactly one of aURI and aChannel must be null. >+ virtual nsresult Load(nsIURI* aURI, >+ nsIChannel* aChannel, >+ nsIStreamListener **aListener) = 0; Document that aListener must be non-null if and only if aChannel is non-null? >+ /** >+ * Create a channel for the stream, reading data from the >+ * media resource at the URI. Call on main thread only. >+ * @param aChannel if non-null, this channel is used and aListener >+ * is set to the listener we want for the channel >+ */ >+ nsresult Open(nsMediaDecoder* aDecoder, nsIURI* aURI, >+ nsIChannel* aChannel, nsIStreamListener** aListener); Document that aURI must be the URI of the channel if aChannel is non-null? Or is it ok to pass in some random URI and also a channel? That said, see comments below on the GetURI call. >+++ b/content/media/video/src/nsMediaStream.cpp >- rv = mChannel->Open(getter_AddRefs(mInput)); .... >+ // Open the file this way instead of via nsIChannel::Open since >+ // the channel may already be open >+ rv = NS_NewLocalFileInputStream(getter_AddRefs(mInput), file); Strictly speaking, the two are not equivalent. In particular, they'll behave differently if the file is a .url file on Windows or .desktop file on Linux. That seems a bit suboptimal. Can't we just condition the opening of the channel on aStreamListener as we do elsewhere? >+++ b/content/media/video/src/nsOggDecoder.cpp >+nsresult nsOggDecoder::Load(nsIURI* aURI, nsIChannel* aChannel, >+ aChannel->GetURI(getter_AddRefs(mURI)); Is that the URI you want? For example, if the channel is a chrome channel, do you want the chrome:// URI here, or the jar:file:// URI? >+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp >@@ -471,17 +471,20 @@ static nsDefaultMimeTypeEntry defaultMim >+ { "audio/ogg", "oga" }, >+ { "video/ogg", "ogv" }, >+ { "audio/ogg", "ogg" } These should, imo, go in the extra list, not the "override everything" list.

(In reply to comment #17) > (From update of attachment 345065 [details] [diff] [review]) > >+++ b/content/html/content/public/nsHTMLMediaElement.h > >+ PRBool CreateDecoder(const nsACString& aType); > > Document what the return value means? And maybe aMIMEType? > > Also document the other new functions here? Sure > >+++ b/content/html/content/src/nsHTMLMediaElement.cpp > > NS_IMETHODIMP nsHTMLMediaElement::Load() > > { > >+ return LoadWithChannel(nsnull, nsnull); > >+} > >+ > >+nsresult nsHTMLMediaElement::LoadWithChannel(nsIChannel *aChannel, > >+ nsIStreamListener **aListener) > >+{ > >+ *aListener = nsnull; > > Won't that crash if someone calls load()? Please add a test for this! Er, yes. I'm sure the existing tests would have caught that, had I run them. > >+PRBool nsHTMLMediaElement::CanHandleMediaType(const char* aMIMEType) > >+{ > >+#if MOZ_OGG > > #ifdef? Right > >+void nsHTMLMediaElement::InitMediaTypes() > >+ catMan->AddCategoryEntry("Gecko-Content-Viewers", gOggTypes[i], > >+ "@mozilla.org/content/document-loader-factory;1", > >+ PR_TRUE, PR_TRUE, nsnull); > > Would it make sense to pass PR_FALSE for aPersist like SVG does? Maybe. I'm not sure what difference it would make in practice. I copied this from the image handler code. > >+nsresult nsHTMLMediaElement::PickMediaElement() > > if (HasAttr(kNameSpaceID_None, nsGkAtoms::src)) { > > if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) { > > Why do we do that HasAttr check? > > > const nsAFlatCString &charset = doc->GetDocumentCharacterSet(); > > nsContentUtils::NewURIWithDocumentCharset? Probably should do those simplifications in a separate bug. > >+++ b/content/html/document/src/nsVideoDocument.cpp > >+ * Portions created by the Initial Developer are Copyright (C) 1998 > > 2008? And fix the name of the original developer? Bleah yeah > >+nsVideoDocument::StartDocumentLoad(const char* aCommand, > >+ element->SetAutoplay(PR_TRUE); > >+ element->SetControls(PR_TRUE); > >+ element->LoadWithChannel(aChannel, aListener); > >+ > >+ return body->AppendChildTo(element, PR_FALSE); > > Do you need to tell the element the src somewhere here? Otherwise, how will it > know what it needs to reload if it needs to reload? Or how will context menus, > etc, know the right URI? It gets it from the channel... > >+++ b/content/media/video/public/nsMediaDecoder.h > >+ // Start downloading the video. Decode the downloaded data up to the > >+ // point of the first frame of data. > >+ // Exactly one of aURI and aChannel must be null. > >+ virtual nsresult Load(nsIURI* aURI, > >+ nsIChannel* aChannel, > >+ nsIStreamListener **aListener) = 0; > > Document that aListener must be non-null if and only if aChannel is non-null? OK > >+ /** > >+ * Create a channel for the stream, reading data from the > >+ * media resource at the URI. Call on main thread only. > >+ * @param aChannel if non-null, this channel is used and aListener > >+ * is set to the listener we want for the channel > >+ */ > >+ nsresult Open(nsMediaDecoder* aDecoder, nsIURI* aURI, > >+ nsIChannel* aChannel, nsIStreamListener** aListener); > > Document that aURI must be the URI of the channel if aChannel is non-null? Or > is it ok to pass in some random URI and also a channel? No, I'll document that. > >+++ b/content/media/video/src/nsMediaStream.cpp > >- rv = mChannel->Open(getter_AddRefs(mInput)); > .... > >+ // Open the file this way instead of via nsIChannel::Open since > >+ // the channel may already be open > >+ rv = NS_NewLocalFileInputStream(getter_AddRefs(mInput), file); > > Strictly speaking, the two are not equivalent. In particular, they'll behave > differently if the file is a .url file on Windows or .desktop file on Linux. > > That seems a bit suboptimal. Can't we just condition the opening of the > channel on aStreamListener as we do elsewhere? We need a stream we can QI to nsISeekableStream. We can only get that by reopening the channel, as far as I can tell ... but you can only open a channel once, right? And it's already open. So I don't know what to do here. > >+++ b/content/media/video/src/nsOggDecoder.cpp > >+nsresult nsOggDecoder::Load(nsIURI* aURI, nsIChannel* aChannel, > >+ aChannel->GetURI(getter_AddRefs(mURI)); > > Is that the URI you want? > > For example, if the channel is a chrome channel, do you want the chrome:// URI > here, or the jar:file:// URI? The original URI I guess. So I should be using nsIChannel::GetOriginalURI. > >+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp > >@@ -471,17 +471,20 @@ static nsDefaultMimeTypeEntry defaultMim > >+ { "audio/ogg", "oga" }, > >+ { "video/ogg", "ogv" }, > >+ { "audio/ogg", "ogg" } > > These should, imo, go in the extra list, not the "override everything" list. OK.

The changes to nsMediaStream, nsChannelReader, etc to pass the listener down look fine. I did notice while testing if I go to a page that contains a <video src=foo.ogg> and a link to the foo.ogg file that is a direct link, and I click on the direct link, nothing seems to happen. eg: http://tinyvid.tv/show/10fhdikwpnknl On that page, click the 'Download this video'. The throbber goes but I'm not taken to a page showing the video.

> Maybe. I'm not sure what difference it would make in practice I think it only matters when switching back and forth between builds that do and don't have the relevant code compiled in.... > It gets it from the channel... That doesn't help context menus. You could set the attrs on the node with aNotify = PR_FALSE, no? > We need a stream we can QI to nsISeekableStream. We can only get that by > reopening the channel, as far as I can tell ... but you can only open a > channel once, right? And it's already open. So I don't know what to do here. There are two cases when we get into this code: channel already open, or not. If it's already open, then its file is the file you really want (with all the redirect things already handled) and using NS_NewLocalFileInputStream is fine. If it's not already open, then you want to call Open() to handle the redirect stuff right, I think. You should be able to condition this on aStreamListener. > The original URI I guess. So I should be using nsIChannel::GetOriginalURI. That'll give the pre-redirect URI for HTTP redirects, which you don't want. Sounds like you want NS_GetFinalChannelURI here.

> > It gets it from the channel... > That doesn't help context menus. You could set the attrs on the node with > aNotify = PR_FALSE, no? The context menu uses the currentSrc DOM attribute to get the URL, not the 'src' attribute from the element. currentSrc represents the URL that the video has selected to play based on the selection algorithm it uses (src attribute, source elements, etc).

Ah, ok. As long as the video element is reporting the right URI then, it's all good.

Hmm, so if we get the file from the channel and it's a .url or whatever file, then the existing code in nsFileStreamStrategy::Open that gets the file size from the file is wrong. That also means we might not get an actual seekable stream, if the .url file redirected to HTTP say. So in fact nsFileStreamStrategy might not know the file size or be able to seek and we really should be using nsHttpStreamStrategy for that case. Right? But how do we detect that?

> the existing code in nsFileStreamStrategy::Open that gets the file size > from the file is wrong. Yeah, looks like it. > That also means we might not get an actual seekable stream Yep. You should be able to QI to seekable stream to see whether what you got is seekable... You should also be able to get the total size from the stream instead of the file. One thing you can probably do is install yourself as a redirect observer on the channel so that you'll know if it got redirected and use the post-redirect channel. But you won't be able to tell ahead of time if you're calling Open(). Basically, Open() is just not very well supported. :(

It sounds like the right thing to do is just what I said in comment #16 --- move the channel opening out to nsHTMLMediaElement, and have all the stream APIs take an async-opened channel, and add whatever magic is needed to ensure that the post-redirect channel is passed into nsMediaStream. But I think that should be a followup bug.

OK. This has the comments addressed as discussed. I've also added a mochitest.

Comment on attachment 345327 [details] [diff] [review] patch v2 Looks good.

Pushed e8cd2199cf0c

Sorry to spam this bug, but considering there are no playback controls (just a blank page in the case of audio) and no streaming capability (bug 455654), as evident when trying to play the following stream: http://live.urn1350.net:8080/urn_high.ogg, this doesn't seem very useful.

There are controls for audio; they fade in if you mouse over the top left area of the page. I suppose we should fix that somehow so controls are always visible (perhaps by creating an <audio> element in that case and fixing the <audio controls> bug). I'll file a separate bug for that. Bug 455654 is going be fixed separately.

Filed bug 462368 to always show the controls bar if a <video> element has no video track. Filed bug 462372 to fix the problems with synchronous Open and file channels redirecting to other kinds of channels. Filed bug 462373 to do the code simplification that Boris pointed out.