We can't open rtsp://xxx.xxx.xxx.xxx to watch rtsp streaming in the browser. The browser returns protocol isn't associated with any problem. We should either launch a video player or play it directly in the browser.

Reference: bug 506834.

Because Vincent is actually studying RTSP protocol, assign this case to him for now.

We have to decide whether to make RTSP part of the Web platform or treat it as some private B2G-only feature. I think probably the latter, but I think this is worth discussing in public in dev.media.

(In reply to Vincent Chang[:vchang] from comment #0) > We can't open rtsp://xxx.xxx.xxx.xxx to watch rtsp streaming in the browser. > The browser returns protocol isn't associated with any problem. We should > either launch a video player or play it directly in the browser. Do we have a chance to see <video src="rtsp://......"></video> within this implementation? That would be very great. And I personally would like to see RTSP support in web platform instead of b2g only. RTSP streaming is used widely in IP Camera/Surveillance, which is usually using web page as their user interface. It would give developers much love if they don't need to create an additional plugin for firefox to display the streaming.

(In reply to Alive Kuo [:alive] (Life is a struggle.) from comment #4) > Do we have a chance to see <video src="rtsp://......"></video> within this > implementation? That would be very great. > And I personally would like to see RTSP support in web platform instead of > b2g only. RTSP streaming is used widely in IP Camera/Surveillance, which is > usually using web page as their user interface. It would give developers > much love if they don't need to create an additional plugin for firefox to > display the streaming. That sounds like something worth discussing on dev.media :-). Vincent, if you're actually working on RTSP support, it would be great to post on dev.media to mention that too :-).

Hi all, the attachment is the draft patch for RTSPMediaResource. https://wiki.mozilla.org/Gecko:RTSPProtocol The major tasks of RTSPMediaResource are: 1. implement MediaSource for OmxDecoder |class RTSPMediaSource : public MediaSource| 2. hold a MediaStreamController that can do play pause seek ... |mMediaStreamController = new MediaStreamController(aChannel);|

The requirements here look a lot like DASH. This should work the same way DASH does. Chris D and Chris P can help you with that.

> Vincent, if you're actually working on RTSP support, it would be great to > post on dev.media to mention that too :-). Hi roc, I am working on protocol part of RTSP streaming support while bechen is working on media decoder part. We are spending much time to pick up the background knowledge before we can really implement it. :-( Basically, I am going to use android's rtsp implementation in libstagefright first. It means this feature will be limited to B2G only in the mean time. The stagefright covers rtsp protocol and rtp/rtcp implementation. The nsRtspChannel will be the interface to libstagefright and runs in main thread of chrome process. nsRtspChannelParent is also running in main thread of chrome process and is the IPC interface to nsRtspChannelChild running in the content process. rtsp in libstagefright(chrome process with multiple android threads) <-> nsRtspChannel(chrome process in the main thread) <-> nsRtspChannelParent(chrome process in the main thread) <-> nsRtspChannelChild(content process) <-> Media Element(content process)

Adding Steve Workman to CC; he did the DASH work, so may have some insight to the decisions that need to be made here.

I looked over the patch and the design and seems to be on the right track from what I can tell - good job. I can look over more patches as they come in. Some thoughts at this early stage: -- It would be great if a library could be found to package with Fx for RTSP - that way we have one well-defined interface that RTSPMediaResource uses. I know that is longer term, but it could help to influence decisions made about class interfaces now. -- It would be great if all of this RTSP work was completely separate from the decoders - I'm not sure how possible that is. For example, you mentioned you need to seek based on time not bytes - In the media engine, the conversion from time to offset happens in one of the subclasses of MediaDecoderReader, which calls MediaResource::Seek. I don't know about OmxDecoder, but I know that WebMReader uses libnestegg to do that conversion. So, my point is that you might need to make changes in whatever OMX Reader class is already there - some change so that the reader knows whether to request a seek by offset or time. That's not a big deal, but it might mean you need to decide which decoders to support first. (it would be great if you could support them all, assuming they all like RTSP :) ) -- Also wondering how you see the networking stack integrate with existing stacks. I'm not up-to-date on WebRTC, but that last I knew, they were using SCTP/UDP. RTSP runs over UDP, right, so it might be helpful to connect with them for some advice too. I don't think there's much overlap with the HTTP/TCP side of things … maybe the DNS resolver? Anyway, just some thoughts. Ping me or email me if you want to discuss more of what I learned from implementing DASH support.

1. Create the rtsp protocol. 2. Hook the allocate/deallocate function to necko

1. create ipdl for ipc 2. implement nsIChannel and nsIStreamListener 3. added nsRtspChannel/nsRtspChannelChild/nsRtspChannelParent

1. move rtsp source code from android framework to gecko 2. create the nsRtspSource, it is the interface to control and receive rtp packets by using rtsp in libstagefright.

media stream controller interface which is used to control the media stream and retrieve meta data.

These patches still need a lot of efforts. But basically, media element can start to load a rtsp source, the nsRtspHander helps to create the RTSPChannelChild and calls AsyncOpen. The AsyncOpen is passed to chrome process's RTSPChannelParent via IPC, the RTSPChannelParent receives ipc request and creates the RTSPChannel and uses nsRtspSource and URI to start RTSP session. If you capture the packets using wireshark, you can see the RTSP session is created. The SDP is exchanged via RTSP session, and server starts to send RTP packets to our device. The RTP packets are accumulated as a frame and passes to RTSPSource.

Comment on attachment 721137 [details] [diff] [review] WIP, add rtsp protocol and ipc Review of attachment 721137 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks fine. As in ready to land once the other patches are ready.

Comment on attachment 721141 [details] [diff] [review] WIP, add ipdl and rtsp channel, rtsp handler Review of attachment 721141 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly cut and paste from WebSocket infrastructure AFAICT. Looks good mostly, a few questions/suggestions ::: netwerk/protocol/rtsp/nsRtspChannel.cpp @@ +75,5 @@ > + > +NS_IMETHODIMP > +RTSPChannel::AsyncOpen(nsIStreamListener *aListener, nsISupports *aContext) > +{ > + LOG("RTSPChannel::AsyncOpen()"); typically want to printf %p here with channel pointer so you can disambiguate which channel is which in the log. @@ +83,5 @@ > + LOG("RTSPChannel::OnStartRequest()\n"); > + mListener->OnStartRequest(this, aContext); > + LOG("RTSPChannel::OnStopRequest()"); > + mListener->OnStopRequest(this, aContext, 999); > + mListener->OnDataAvailable(this, aContext, NULL, 0, 0); I don't know RTSP at all, but it seems *very* weird to be calling OnStopRequest before OnDataAvailable. Generally the nsIChannel model is to call OnStart once, OnData from 0 to many times, then OnStop once. Also this code isn't sending any data--is this just scaffolding until you have real logic? @@ +115,5 @@ > +// BaseRTSPChannel::nsIStreamListener > +//------------------------------------------------------------------------- > +NS_IMETHODIMP > +RTSPChannel::OnDataAvailable(nsIRequest *aRequest, > + nsISupports *aContext, lots of trailing whitespace in this function decl ::: netwerk/protocol/rtsp/nsRtspChannel.h @@ +48,5 @@ > + nsresult aStatusCode); > + NS_IMETHOD OnDataAvailable(nsIRequest *aRequest, > + nsISupports *aContext, > + nsIInputStream *aInputStream, > + uint64_t aOffset, trailing whitespace here and elsewhere. Probably other places in patch too but this is last time I'll note it :) ::: netwerk/protocol/rtsp/nsRtspChannelChild.cpp @@ +23,5 @@ > +namespace net { > + > +NS_IMPL_ADDREF(RTSPChannelChild) > + > +NS_IMETHODIMP_(nsrefcnt) RTSPChannelChild::Release() You may not need this complicated Release override. HTTP uses it because we sometimes have to keep a channel around longer to get security info. I'm not sure why Websocket uses it. For simple protocols like FTP we just have the child call Send__delete() in OnStopRequest. If you don't need the IPC connection after OnStopRequest that's probably the way to go. See FTPChannelChild.cpp's OnStopRequest implementation @@ +220,5 @@ > +RTSPChannelChild::RecvOnTransportAndData(const nsCString& aMsg, const uint32_t & count, const uint8_t& pt) > +{ > + LOG("RTSPChannelChild::RecvOnTransportAndData() aMsg = %s length = %d", aMsg.get(), count); > + if (mEventQ.ShouldEnqueue()) { > + mEventQ.Enqueue(new MessageEvent(this, aMsg, count, pt)); If RTSP doesn't ever cause the event loop to be spun on the child (doesn't call into JS that might do things like a sync XHR, etc), then we might be able to get away without queing. Probably safer to keep queuing on for now. Sorry there's so much boilerplate to copy/paste for this stuff. @@ +267,5 @@ > + return NS_ERROR_ILLEGAL_VALUE; > + } > + > + LOG("RTSPChannelChild::AsyncOpen(2)"); > + // Corresponding release in DeallocPWebSocket DeallocPWebsocket, eh? :P @@ +275,5 @@ > + IPC::SerializedLoadContext(this)); > + > + LOG("RTSPChannelChild::AsyncOpen(3)"); > + SerializeURI(mURI, uri); > + SendAsyncOpen(uri); I'm merging the constructor and asyncOpen calls into one IPDL message in bug 558623. If that lands before this take a look and see if you can copy the approach in your patches. Or it can be a followup. ::: netwerk/protocol/rtsp/nsRtspChannelParent.cpp @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsBaseRtspChannel.h" > +#include "nsRtspChannelParent.h" The convention now for new code is to eliminate the 'ns' prefix in file names. So BaseRtspChannel, RtspChannelParent, RtspChannel, etc. @@ +44,5 @@ > +{ > + LOG("RTSPChannelParent::RecvDeleteSelf() %p\n", this); > + mChannel = nullptr; > + mAuthProvider = nullptr; > + return mIPCOpen ? Send__delete__(this) : true; Related to the :Release comment: From this snippet I suspect that you don't need the back and forth here (SendDeleteSelf sent by child to parent, then parent sends _delete_ to child). You can probably just having child send__delete, and then the parent's destructor releases mChannel. I'm also not clear about the "return mIPCOpen ?" here. If IPC isn't open this function wouldn't get called. Don't think you need it. Was that copied from some code somewhere? @@ +47,5 @@ > + mAuthProvider = nullptr; > + return mIPCOpen ? Send__delete__(this) : true; > +} > + > +bool nit: lots of trailing whitespace after the return types of your functions @@ +178,5 @@ > +RTSPChannelParent::GetInterface(const nsIID & iid, void **result) > +{ > + LOG("RTSPChannelParent::GetInterface()"); > + if (mAuthProvider && iid.Equals(NS_GET_IID(nsIAuthPromptProvider))) > + return mAuthProvider->GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL, Does RSTP handle HTTP basic auth, NTLM, etc? If not you don't need to implement nsIAuthPromptProvider. ::: netwerk/protocol/rtsp/nsRtspChannelParent.h @@ +21,5 @@ > +namespace net { > + > +class RTSPChannelParent : public PRTSPChannelParent > + , public nsIInterfaceRequestor > + , BaseRTSPChannel Why are you having RTSPChannelParent implement BaseRTSPChannel? If we use the same basic setup as our other necko IPDL classes, you'd have (ns)RtspChannel and RtspChannelChild inherit from the base channel, but not the parent. ::: netwerk/protocol/rtsp/nsRtspHandler.cpp @@ +36,5 @@ > +{ > + nsRtspHandler* ph = new nsRtspHandler(); > + LOG("nsRtspHandler::Create()\n"); > + if (ph == nullptr) > + return NS_ERROR_OUT_OF_MEMORY; Don't need to check for out of memory any more: https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation Just calling new is enough.

Comment on attachment 721141 [details] [diff] [review] WIP, add ipdl and rtsp channel, rtsp handler Review of attachment 721141 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/rtsp/nsRtspChannel.cpp @@ +83,5 @@ > + LOG("RTSPChannel::OnStartRequest()\n"); > + mListener->OnStartRequest(this, aContext); > + LOG("RTSPChannel::OnStopRequest()"); > + mListener->OnStopRequest(this, aContext, 999); > + mListener->OnDataAvailable(this, aContext, NULL, 0, 0); I see in the later patch that you're replacing this with real code, no so I consider myself enlightened on this point.

can't be compiled. - create RTSPMediaResource - receive data from |RTSPMediaResource::OnDataAvailable| - Omx get data through |ReadTrack()|, called by |RTSPMediaSource::read| (another patch) - |GetMediaStreamController()|, called by |OmxDecoder::InitRTSP()| (another patch) - |SeekTime()|, called by |RTSPReader::Seek| (another patch)

can't be compiled - create RTSPDecoder, RTSPReader - |class RTSPMediaSource : public MediaSource| in OmxDecoder.cpp - |OmxDecoder::InitRTSP()|

Comment on attachment 721144 [details] [diff] [review] WIP, rtsp in libstagefright Review of attachment 721144 [details] [diff] [review]: ----------------------------------------------------------------- Cool - making progress :) This looks like you're working on creating the connection first, and will work on getting data etc. later. Which seems like a good plan. In general, it seems like you on the right tracks connecting RtspSource to RtspChannel. I don't know very much about the RTSP code you're pulling in from libstagefright, so some of my comments may be wrong. It would be good if you could isolate those in a separate patch. And if you make any changes to them, please attach a diff comparing the libstagefright originals to the new Mozilla ones. I have some other comments on naming conventions and file layout, but it definitely seems like the right direction for these files. Good work. ::: netwerk/protocol/rtsp/nsRtspChannel.cpp @@ +81,5 @@ > + if (!mURI) { > + LOG("RTSPChannel::AsyncOpen() illegal URI"); > + return NS_ERROR_ILLEGAL_VALUE; > + } > + nsCString uriSpec; nsAutoCString is good for local strings. ::: netwerk/protocol/rtsp/nsRtspSource.cpp @@ +63,5 @@ > +} > + > +void RTSPSource::start() > +{ > + printf_stderr("RTSPSource::start()\n"); I suggest you create a pr_log for RTSP; maybe gRTSPLog, rather than printf_stderr; @@ +70,5 @@ > + mLooper = new ALooper; > + mLooper->setName("rtsp"); > + mLooper->start(); > + > + mReflector = new AHandlerReflector<RTSPSource>(this); I don't know the libstagefright RTSP API, so you should get someone who does to look over these parts :) @@ +74,5 @@ > + mReflector = new AHandlerReflector<RTSPSource>(this); > + mLooper->registerHandler(mReflector); > + } > + > + printf_stderr("RTSPSource::start() 1\n"); '1' is fine, but please add a descriptive statement as well. @@ +75,5 @@ > + mLooper->registerHandler(mReflector); > + } > + > + printf_stderr("RTSPSource::start() 1\n"); > + CHECK(mHandler == NULL); Is CHECK an Android or B2G specific macro? If not, please change these to NS_ASSERTION or NS_ENSURE_TRUE. @@ +80,5 @@ > + > + sp<AMessage> notify = new AMessage(kWhatNotify, mReflector->id()); > + > + printf_stderr("RTSPSource::start() 2\n"); > + mHandler = new MyHandler(mURL.c_str(), notify, mUIDValid, mUID); What is mHandler handling? Is it the connection? Or the RTSP session? Please expand the name to something more descriptive. @@ +115,5 @@ > +void RTSPSource::onMessageReceived(const sp<AMessage> &msg) > +{ > + printf_stderr("RTSPSource::onMessageReceived()\n"); > + > + if (msg->what() == kWhatDisconnect) { Can you refactor these k vars to ALL CAPS? It seems like they are better described as values of state (usually ALL_CAPS), rather than constant values (kConstantVar). @@ +144,5 @@ > + int32_t what; > + CHECK(msg->findInt32("what", &what)); > + > + switch (what) { > + case MyHandler::kWhatConnected: It looks like these are a second 'what', i.e. 1st 'what' is kWhatNotify, and 2nd 'what' is being switched on here. If that's correct, can you add a prefix to the var names, something like kNotifyConnected (or better, MSG_NOTIFY_CONNECTED). Also, this is a long function - better to break it up. I'd start with adding OnNotificationMessageReceived, and I'd strongly consider adding others like OnNotifyAccessUnit. This will help with readability, and might help identify possible code reuse later. @@ +309,5 @@ > + mTracks.push(info); > + } > +*/ > + > + mState = CONNECTED; Is this all going to be on the same thread, or do you need a monitor to protect accesses to mState (and maybe other variables)? ::: netwerk/protocol/rtsp/nsRtspSource.h @@ +10,5 @@ > +#include <media/stagefright/foundation/AString.h> > +#include <utils/RefBase.h> > +#include <media/stagefright/foundation/AHandlerReflector.h> > + > +namespace android { Any reason you're using the android namespace? Why not mozilla::net::? @@ +16,5 @@ > +struct ALooper; > +//struct AnotherPacketSource; > +struct MyHandler; > + > +class RTSPSource : public RefBase Keep the class name and filename the same. If you want it to match the rest of mozilla::net, then nsRTSPSource is probably a good idea. @@ +25,5 @@ > +// const KeyedVector<String8, String8> *headers, > + bool uidValid = false, > + uid_t uid = 0); > + > + void start(); Please capitalize first letter of function names. ::: netwerk/protocol/rtsp/rtsp/MyHandler.h @@ +54,5 @@ > + > +namespace android { > + > +static void MakeUserAgentString(AString *s) { > + s->setTo("stagefright/1.1 (Linux;Android "); Hmm. I'm not sure if stagefright is the right UA to use. You might want to discuss this with someone who is more involved with the UA, but I think it should be the FirefoxOS UA. I could be wrong - check it out. @@ +94,5 @@ > + s = colonPos + 1; > + } > +} > + > +struct MyHandler : public AHandler { Is this an Android file, or one you wrote yourself? MyHandler is a very generic name. If it's a libstagefright file and you can't change the contents, then you can ignore a lot of my comments. @@ +160,5 @@ > + > + mSessionHost = host; > + } > + > + void connect() { There are a lot of inline function implementations here. If you can, add a cpp file, please. @@ +1227,5 @@ > + int32_t mKeepAliveGeneration; > + > + Vector<TrackInfo> mTracks; > + > + void setupTrack(size_t index) { Variables at the end of the class definition please :)

Hi Jason and Steve, Thanks for your help to review the patches. I am about to create the new patches to address your review comments and add the patch for media stream controller this week. The rtp stream and meta data will be passed from chrome process to content process via new listener interface and media stream control functions will move to the media stream controller. Before post the new patches, I would like to do the integration with bechen's patches. Hopefully, we could watch the rtsp stream after integration. :-) Regards Vincent

Comment on attachment 721149 [details] [diff] [review] WIP, media stream controller interface Vincent, I'm removing the f? here since you said you'll write new patches. I know I didn't look over this one, but I think the comments on the other patch should help enough for now. Let me know if you still want this feedback.

Comment on attachment 721616 [details] [diff] [review] wip, RtspMediaResource Benjamin, sorry I didn't get around to providing feedback. Do you still need it or are you working on new patches? I'm going to remove the f? and you can add it again if you still want feedback from me for these versions of the patches.

refactor the patch due to new file moz.build

1. The rtsp source will be created in mediastreamcontroller. 2. Move the control functions to mediastreamcontroller. 3. remove trail blank ...

The source comes from frameworks/base/media/libstagefright/rtsp in B2G tree.

This patch includes the IPC code and new interfaces.

compiler flag for RTSP

You can decompress rtsp.zip and put to gaia/apps/ if you want to test RTSP patches.

Hi Jason and Steve, finally making progress. Wow... really exciting to see the RTSP streaming works on unagi device. You can apply these eight patches and gaia app and see how it works. Basically, we can support video tag with RTSP source right now. <video> <source src="rtsp://xxxxxx"/> </video> The next steps for us are 1. Add the play/seek/pause/stop function. 2. Fix the crash issues in various of conditions(such as kill the apps) 3. Support more audio/video codecs ... 4. Refactor the patches. ...

Comment on attachment 727581 [details] [diff] [review] Patch 2, RTSP decoder Review of attachment 727581 [details] [diff] [review]: ----------------------------------------------------------------- Just some a few little things I spotted. You've done a lot of good work, well done! ::: content/media/MediaDecoder.cpp @@ +1660,5 @@ > +#ifdef MOZ_RTSP > +bool > +MediaDecoder::IsRTSPEnabled() > +{ > + //return Preferences::GetBool("media.rtsp.enabled"); Looks like you forgot to remove this comment? ::: content/media/omx/MediaOmxReader.h @@ +76,5 @@ > +public: > + RTSPReader(AbstractMediaDecoder* aDecoder); > + ~RTSPReader(); > + > + virtual nsresult Init(MediaDecoderReader* aCloneDonor); You don't need to put the "virtual" keyword when overriding a method in C++, but please add MOZ_OVERRIDE at the end, i.e.: nsresult Init(MediaDecoderReader* aCloneDonor) MOZ_OVERRIDE; Thanks. ::: content/media/omx/OmxDecoder.cpp @@ +571,5 @@ > + if (maxWidth > 0 && maxHeight > 0 && > + !(videoSource->getFormat()->findInt32(kKeyWidth, &width) && > + videoSource->getFormat()->findInt32(kKeyHeight, &height) && > + width * height <= maxWidth * maxHeight)) { > + printf_stderr("Failed to get video size, or it was too large for HW decoder (<w=%d, h=%d> but <maxW=%d, maxH=%d>)", Use NSPR logging, not printf logging, like we do here: http://mxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder.cpp#378 (I'm not sure if you know how to turn this on, set NSPR_LOG_MODULES=OmxDecoder:5 environment variable)

Regarding to RTSPDeocder/ RTSPReader/ RTSPMediaStream 1. The reason that you need to put "RTSP" sematic into OmxDecoder is because you need a new Extractor which wrap RTSP controller inside to provide track information & RTSPMediaSoruce object creation. I would suggest create a new RTSPExtractor and reuse OmxDecoder, instead of create RTSPDecoder, which duplicate most code of OmxDecoder. 2. RTSPReader may inherit from OmxReader. The only different between RTSPReader & OmxReader is RTSPReader need to pass RTSPExtractor into OmxDecoder for demul.

Comment on attachment 727580 [details] [diff] [review] Patch, compiler flag Review of attachment 727580 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but get someone like :ted who is more familiar with configure.in to have a look. ::: configure.in @@ +4598,5 @@ > ;; > > cairo-gonk) > AC_DEFINE(MOZ_WIDGET_GONK) > + AC_DEFINE(MOZ_RTSP) Seems fine, but you should have someone more familiar with configure.in to review this (:ted?). Also, you might want to consider a --enable-rtsp flag for the build - see other examples in configure.in. But I'd only do this if you think that you will be working on the code for a long time before you want to enable the build by default.

Comment on attachment 727582 [details] [diff] [review] WIP 2, RTSP mediaresource and media element Review of attachment 727582 [details] [diff] [review]: ----------------------------------------------------------------- Overall going in the right direction - good work - I just have some ideas and questions. One general thing: it would be good to have an overall comment explaining how HTMLMediaElement, RTSPMediaResource, the decoder, controllers etc. all work together. There was something like this added to DASHDecoder.cpp, and I tried to refer to it in the other files. It should help comprehension for the reviews now and for maintenance later. ::: content/html/content/public/HTMLMediaElement.h @@ +59,5 @@ > typedef mozilla::MediaStream MediaStream; > typedef mozilla::MediaResource MediaResource; > typedef mozilla::MediaDecoderOwner MediaDecoderOwner; > typedef mozilla::MetadataTags MetadataTags; > + friend class mozilla::RTSPMediaResource; I'd prefer to see you make the functions you want to call public, and in the case of FinishDecoderSetup, add an overloaded version which means you don't need to pass two nullptrs. Then you can keep the current FinishDecoderSetup private. You might be able to create other such public wrapper functions. ::: content/html/content/src/HTMLMediaElement.cpp @@ +344,5 @@ > element && > NS_SUCCEEDED(rv = element->InitializeDecoderForChannel(channel, getter_AddRefs(mNextListener))) && > mNextListener) { > rv = mNextListener->OnStartRequest(aRequest, aContext); > } else { I understand that you need to do something different for RTSP streams here, but it looks like you could merge the two branches which call InitializeDecoderForChannel - they both check "if (channel && element)". Also, the code already gets the mimetype in InitializeDecoderForChannel, so you can do the check there, and return *aListener as nullptr. Then, "if (mNextListener) ...". @@ +2398,5 @@ > > // stream successfully created, the stream now owns the channel. > mChannel = nullptr; > > + if (mimeType.EqualsLiteral("RTSP")) { HTMLMediaElement is mostly codec agnostic in it's own code - decisions about which decoder to create are in DecoderTraits. I suggest you add another trait function to determine if FinishDecoder should be called ... something like DecoderTraits::DecoderWaitsForOnConnected, or whatever is appropriate. But my point is to keep the codec/mimetype side of things out of HTMLMediaElement. Also, since you call SetResource and set mDecoder = decoder, add more to you comment to explain that you're setting the resource and decoder now, before postponing. You might also need to consider what decoder functions HTMLMediaElement might call if mDecoder is not null; in the current code, it will only call said functions once FinishDecoderSetup is complete, but this code changes that. Make sure to check state inside your RTSP decoder at the start of these functions. Similarly, make sure to check state as well as mResource inside your decoder. ::: content/media/MediaResource.cpp @@ +1244,5 @@ > > +#define BUFFER_SLOT_NUM 128 > +#define BUFFER_SLOT_SIZE 8192 > + > +class RTSPTrackBuffer A comment to explain this class, please. And comments for its functions and member variables. @@ +1382,5 @@ > + nsIInputStream* aStream, > + uint64_t aOffset, > + uint32_t aCount) > +{ > + return NS_OK; If this is all this function does, you can put that in the header file. @@ +1386,5 @@ > + return NS_OK; > +} > + > +nsresult > +RTSPMediaResource::Listener::OnDataAvailable(uint8_t index, OnDataAvailable is a well known function, but the parameters are different in this overloaded version. Can you add a one line comment to state that it is different from nsIStreamListener's one, and where it is declared. @@ +1479,5 @@ > + return NS_ERROR_DOM_BAD_URI; > + } > + }*/ > + > + // TODO: RTSP channel Isn't there already a channel by this stage? What RTSP channel would you create here? @@ +1587,5 @@ > + return NS_OK; > +} > + > +nsresult > +RTSPMediaResource::OnDisConnected(uint8_t index, uint32_t reason) OnDisconnected, small 'c'. ::: content/media/MediaResource.h @@ +665,5 @@ > + const nsACString& aContentType); > + ~RTSPMediaResource(); > + > + // TODO: > + // assume that we only have one audio track Why? @@ +670,5 @@ > + // create them at |RTSPMediaResource::OnStartRequest| > + nsCOMPtr<nsIMediaStreamController> mMediaStreamController; > + nsTArray<RTSPTrackBuffer*> mTrackBuffer; > + > + virtual nsIMediaStreamController* GetMediaStreamController() MOZ_OVERRIDE { You only need to specify virtual in the base class; thanks for putting MOZ_OVERRIDE here. @@ +719,5 @@ > + virtual int64_t GetCachedDataEnd(int64_t aOffset){return 0;} > + virtual bool IsDataCachedToEndOfResource(int64_t aOffset){return false;} > + > + > +////// copy from ChannelMediaResource If this is a direct copy, and you don't see any changes, you might consider adding it to BaseMediaResource.

Comment on attachment 727581 [details] [diff] [review] Patch 2, RTSP decoder Review of attachment 727581 [details] [diff] [review]: ----------------------------------------------------------------- Seems like good progress. ::: content/media/omx/MediaOmxDecoder.h @@ +20,5 @@ > virtual MediaDecoder* Clone(); > virtual MediaDecoderStateMachine* CreateStateMachine(); > }; > > +class RTSPDecoder : public MediaDecoder I think RTSPDecoder should get its own .h and .cpp files like the other decoders, unless it's going to be linked very closely to MediaOmxDecoder. Even then, all the other decoders based on MediaDecoder have their own files, so I think this one should too. ::: content/media/omx/MediaOmxReader.h @@ +59,5 @@ > virtual nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime); > virtual nsresult GetBuffered(mozilla::dom::TimeRanges* aBuffered, int64_t aStartTime); > }; > > +class RTSPReader : public MediaDecoderReader You can probably give RTSPReader its own files too. ::: content/media/omx/OmxDecoder.cpp @@ +450,5 @@ > > return true; > } > > +bool OmxDecoder::InitRTSP() { Really long function! Please consider splitting it up.

Comment on attachment 727573 [details] [diff] [review] Patch , add Android's RTSP and RTP to gecko Review of attachment 727573 [details] [diff] [review]: ----------------------------------------------------------------- Can you add a general comment to explain this integraion and how the Android APIs work? Then I can provide better feedback. At this stage, though, it seems like it's moving in the right direction. ::: netwerk/protocol/rtsp/Makefile.in @@ +6,5 @@ > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > +VPATH = \ > + $(srcdir) \ > + $(srcdir)/rtsp \ Can you add another Makefile in the rtsp dir and refer to it here, instead of referring to the source files themselves. ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp @@ +39,5 @@ > mFinalResult(OK), > mDisconnectReplyID(0), > + mSeekGeneration(0) > +{ > +} Trailing whitespace ;) ::: netwerk/protocol/rtsp/rtsp/RTSPSource.h @@ +42,4 @@ > bool uidValid = false, > uid_t uid = 0); > > + void start(); If these functions are implementing virtual functions in the base class, please add MOZ_OVERRIDE: void start() MOZ_OVERRIDE;

I'll try to get to the last couple of patches tomorrow. Good work so far!

Comment on attachment 727572 [details] [diff] [review] Patch 2, original Android source code for RTSP and RTP Review of attachment 727572 [details] [diff] [review]: ----------------------------------------------------------------- These files are just copied, right? If you made any changes to them, can you upload a diff showing those changes, please?

Comment on attachment 727575 [details] [diff] [review] Patch 1, mediastreamcontroller implementation Review of attachment 727575 [details] [diff] [review]: ----------------------------------------------------------------- Good progress. I've given feedback on the non e10s files. I've left anything with 'Parent' or 'Child' in it for Jason. Good work - keep it coming :) ::: netwerk/protocol/rtsp/mediastreamcontroller/MediaStreamService.cpp @@ +17,5 @@ > +nsMediaStreamControllerService::nsMediaStreamControllerService() > +{ > +} > + > +/* static */ StaticRefPtr<nsMediaStreamControllerService> nsMediaStreamControllerService::sSingleton; Put the comment above the declaration. @@ +31,5 @@ > + return service.forget(); > +} > + > +NS_IMETHODIMP > +nsMediaStreamControllerService::Create(nsIChannel *channel, nsIMediaStreamController **result) Please use 'aChannel' and 'aResult' - 'a' is for argument. ::: netwerk/protocol/rtsp/mediastreamcontroller/RTSPMediaStreamController.cpp @@ +41,5 @@ > +#include "zlib.h" > +#include <algorithm> > +#include "android/log.h" > + > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "Gonk", args) I think NSPR logging is redirected to the android log, right? If so, please use nspr logging here so that it doesn't need to be converted in the future, when/if RTSP is ported to desktop and mobile. @@ +43,5 @@ > +#include "android/log.h" > + > +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "Gonk", args) > + > +using namespace mozilla; Do you need this if you also enter the mozilla namespace below? @@ +63,5 @@ > + LOG("RTSPMediaStreamController::~RTSPMediaStreamController()"); > +} > + > +NS_IMETHODIMP > +RTSPMediaStreamController::GetTrackMeta(uint8_t index, nsIMediaStreamMetaData * *_retval) GetTrackMetadata? @@ +136,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +RTSPMediaStreamController::OnDataAvailable(uint8_t index, I'm wondering if you should rename this to OnMediaDataAvailable? It's probably not a big deal, but it would clarify that it's not the same OnDataAvailable defined in nsIStreamListener. ::: netwerk/protocol/rtsp/mediastreamcontroller/RTSPMediaStreamMetaData.cpp @@ +56,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +RTSPMediaStreamMeta::SetMimeType(const nsACString & aMimeType) Do you need to do any mime type checking here? Or does that happen further up the stack, in the media engine code? ::: netwerk/protocol/rtsp/mediastreamcontroller/nsIMediaStreamController.idl @@ +90,5 @@ > + /** > + * Called RTSP session is closed. > + * @param > + */ > + void onDisConnected(in uint8_t index, in uint32_t reason); onDisconnected - small 'c'. @@ +97,5 @@ > +/** > + * Media stream controller API: control and retrieve meta data from media stream. > + */ > +[uuid(a9bdd4b0-8559-11e2-9e96-0800200c9a66)] > +interface nsIMediaStreamController : nsISupports I didn't think about this earlier, sorry, but the term 'MediaStream' might be confused with the DOM MediaStream API used in WebRTC. Another possibility could be StreamingProtocolController (which also clarifies that it's about control at the protocol level, not higher up). I know this is an annoying comment, but I'd rather it was thought about now before the code is landed and someone requests it to be refactored. Speak to cpearce and see if he thinks it's necessary to rename it. If you do this, you could also rename RTSPMediaStreamController to just RTSPController. @@ +99,5 @@ > + */ > +[uuid(a9bdd4b0-8559-11e2-9e96-0800200c9a66)] > +interface nsIMediaStreamController : nsISupports > +{ > + void init(in nsIURI aUri); Comment for init, please (and any other methods or attributes you didn't comment in the idl files). ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp @@ +92,5 @@ > } > > +void RTSPSource::play() > +{ > + printf_stderr("RTSPSource::play()\n"); NSPR logs please. ::: netwerk/protocol/rtsp/rtsp/RTSPSource.h @@ +121,5 @@ > void finishDisconnectIfPossible(); > > void performSeek(int64_t seekTimeUs); > > + void OnDataAvailable(size_t trackIndex); I know I asked you to use caps for the first char in function names, but since this is an existing android class, it's probably better to be use the convention that was already established. So, new code and existing moz code, all initial caps (OnDataAvailable). But in changes/additions to imported code, use whatever convention is already there (in this case, onDataAvailable). This is a nitpick, but the class definition will look better.

(In reply to Steve Workman [:sworkman] from comment #42) > Comment on attachment 727572 [details] [diff] [review] > Patch 2, original Android source code for RTSP and RTP > > Review of attachment 727572 [details] [diff] [review]: > ----------------------------------------------------------------- > > These files are just copied, right? If you made any changes to them, can you > upload a diff showing those changes, please? These files are copied from frameworks/base/media/libstagefright/rtsp. The diff will in the "add Android's RTSP and RTP to gecko" and "mediastreamcontroller implementation" patches.

> > +NS_IMETHODIMP > > +NS_IMETHODIMP > > +RTSPMediaStreamMeta::SetMimeType(const nsACString & aMimeType) > > Do you need to do any mime type checking here? Or does that happen further > up the stack, in the media engine code? The mime type is coming from SDP in RTSP server's describe reply message and is passed to media engine in type of RTSPMediaStreamMeta class. > > + * Media stream controller API: control and retrieve meta data from media stream. > > + */ > > +[uuid(a9bdd4b0-8559-11e2-9e96-0800200c9a66)] > > +interface nsIMediaStreamController : nsISupports > > I didn't think about this earlier, sorry, but the term 'MediaStream' might > be confused with the DOM MediaStream API used in WebRTC. Another possibility > could be StreamingProtocolController (which also clarifies that it's about > control at the protocol level, not higher up). I know this is an annoying > comment, but I'd rather it was thought about now before the code is landed > and someone requests it to be refactored. Speak to cpearce and see if he > thinks it's necessary to rename it. > > If you do this, you could also rename RTSPMediaStreamController to just > RTSPController. ok, I'll rename it as StreamingProtocolController and remove *MediaStream*

1, Fix some bugs for play/pause/seek. 2, Address the feedback comments. The MediaStream will be rename to StreamingProtocol.

Address the feedback comments and fix bugs.

fix bug and add some somments

bug fix

Why is this blocked by bug 856542?

(In reply to Chris Double (:doublec) from comment #51) > Why is this blocked by bug 856542? For now, we are using the omx decoder to decode the RTSP streaming. We will encounter the same hardware resource problem eventually. For example: 1. play a RTSP streaming in browser or in other app 2. launch video app to play a h264 file

(In reply to Vincent Chang[:vchang] from comment #53) > Created attachment 737361 [details] > Draft architecture v0.1 Thanks for that - that helps my understanding.

Comment on attachment 727572 [details] [diff] [review] Patch 2, original Android source code for RTSP and RTP Looks fine, since this is just a simple copy.

Comment on attachment 727568 [details] [diff] [review] WIP 2, Add RTSP protocol and IPC Review of attachment 727568 [details] [diff] [review]: ----------------------------------------------------------------- I know this is a pain, but please switch the capitalization from RTSPChannel->RtspChannel (and PRtspChannel to PRtspChannel). That's how most of the other necko classes are named, and this is the time to fix it (before we land it all initially). Other than than this will be +r--looks good.

(In reply to Steve Workman [:sworkman] from comment #55) > Comment on attachment 727572 [details] [diff] [review] > Patch 2, original Android source code for RTSP and RTP > > Looks fine, since this is just a simple copy. Also, probably a good idea to land the RTSP base code now. It will be good for revision history to have the base code there, followed by any changes later. Since this patch is a direct copy, I would go ahead and land it now. If you want to do that, just r? me.

Comment on attachment 735123 [details] [diff] [review] Patch v04 - RTSP Decoder Review of attachment 735123 [details] [diff] [review]: ----------------------------------------------------------------- In general this looks good. Seems clear how the Extractor fits with the controller. One thing though: please keep separate files for each class. It's clear for the revision history in the longer term. Someone from the media team may want to review this.

Comment on attachment 735644 [details] [diff] [review] Patch v04 - RTSP Resource and Media Element Review of attachment 735644 [details] [diff] [review]: ----------------------------------------------------------------- Continuing with a lot of good work here - well done. Here's how I see the current flow of execution - please correct me if I'm wrong: - media element gets an RTSPChannel from RTSPHandler. This involves IPDL messages, because the channel is created in the Chrome/Parent process. - RTSPHandler immediately calls element->OnStartRequest, which is another IPDL message. - Then, RTSPMediaResource is created, which creates an RTSPController object, and calls mMediaStreamController->AsyncOpen. - The controller calls RTSPSource->Start and the android RTSP code opens a socket and sets up the session, passing messages to and fro using ALooper. From this point on, play, seek and other commands are passed through the RTSPOmxReader to RTSPMediaResource and on to RTSPController. This seems good to me. So, RTSPChannel is kind of a dummy channel, right? It's only there to process the URI, and doesn't do much more, right? And low level sockets are created in the content process - so this bypasses a lot of IPDL messages for data transfer (which is a good thing to try to avoid). So, maybe Jason can answer this: in RTSPChannelChild, gNeckoChild is asked to construct an RTSPChannel in the parent process, and then AsyncOpen is sent. Since AsyncOpen doesn't really do anything on the parent, could it be an async IPDL, and could OnStartRequest be called directly from the child? Or does that break IPDL conventions? Seems like some time could be saved at startup this way. Of course, maybe there are plans to do more with RTSPChannel.

(In reply to Steve Workman [:sworkman] from comment #59) > And low level sockets are created in the content process - so this bypasses > a lot of IPDL messages for data transfer (which is a good thing to try to > avoid). > > So, maybe Jason can answer this: in RTSPChannelChild, gNeckoChild is asked > to construct an RTSPChannel in the parent process, and then AsyncOpen is > sent. Since AsyncOpen doesn't really do anything on the parent, could it be > an async IPDL, and could OnStartRequest be called directly from the child? > Or does that break IPDL conventions? Seems like some time could be saved at > startup this way. > > Of course, maybe there are plans to do more with RTSPChannel. Wait - ignore this - I just saw RTSPControllerChild, so my point about avoiding IPDL messages doesn't make sense. Let me come back to this.

Comment on attachment 735121 [details] [diff] [review] Patch part3 v03 - RTSP controller Review of attachment 735121 [details] [diff] [review]: ----------------------------------------------------------------- I may need some help here from smaug or bz or one of the media folks to get the architecture here right. So RTSP channels don't fit into the nsIChannel model (which assumes a simple request/response: i.e. you can only open them with a listener that receives OnStart/OnData/OnStop). So I think RTSP channels should be like websockets, and not implement nsIChannel (and thus not be something you create via nsIIOService/NS_NewChannel). However they do need to implement nsIRequest, so that RTSP channels can be Cancel()'d when the page/document they're part of goes away (navigation, close tab, etc). Websockets do this with a split architecture: there's a DOM class (WebSocket) that implements nsIRequest (to get Cancel), and a netwerk/WebSocketChannel that does all the actual networking. I'm not clear if Rtsp needs to be split up the same way: it probably depends on whether we can ever imagine JS workers using RTSP (if so, we'll want the split). Websockets are instantiated by Javascript. RTSP channels are instantiated by HTMLMediaElements (so :sworkman tells me): https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#1043 I think we should special-case rtsp here, and not use NS_NewChannel. I'm guessing we should pass the rtsp URI to the MediaResourceRTSP (does that live in the child/parent/both?), and have it create a RtspController class (via do_CreateInstance, like we do for WebSocketChannels here): http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#780 The existing RTSPChannel.h/cpp that was added in the "refactor RTSP channel" patch should be completely eliminated (the only thing it appears to exist for is to kick off the MediaResourceRTSP to do the actual work). Assuming we want/need the split architecture for Workers support, we'd have an architecture like Document (in which <media="rstp://" appears) -> MediaResourceRTSP -> RtspDOMClass // implements nsIRequest, like WebSocket.cpp. // Must add to Document's loadGroup // creates RtspNetworkClass and destroys it if Cancel()'d -> RtspNetworkClass // basically what RTSPController classes are // now: i.e. RTSPController plus parent/child for e10s Naming: Not sure what to call RtspDOMClass (RtspDOMElement? That's probably wrong: :smaug would know a good name). I would call the RtspNetworkClass(es) RtspChannel{Child|Parent} (i.e. rename s/RTSPController/RtspChannel, and eliminate the current RTSPChannel classes which we don't need). Does this architecture make sense? It's possible that we don't need a split RtspDOMClass/RtspNetworkClass and we can just have one class for it. Again, that depends on whether there's any DOM-specific stuff that needs to be taken care of and should remain separate from the network code.

roc, does it seem likely to you that we'll ever want to control RTSP from worker JS?

AFAIK we wouldn't want to expose RTSP channels to JS at all.

In that case we can probably just have a single RtspChannel class that implements nsIRequest and nsIStreamingProtocolController.

This all makes sense to me and was part of the reason I was confused the other day. So, what I'm hearing (and what I agree with) is that the existing RtspChannel created in HTMLMediaElement should go away. You'll need to find some way to pass the URI to the RtspMediaResource (other than a callback to OnStartRequest). That seems possible enough - probably another Create method. You might want to add another function to DecoderTraits to determine if a channel should be created or the uri passed in directly - keep it generic for the media element code. And then RtspMediaController should become RtspChannel, like Jason says, implementing nsIRequest, nsIStreamingMediaController and nsIStreamingMediaListener. This feels like a cleaner, clearer architecture.

Hi Jason and Steve, Thanks for your comments, the RtspChannel is designed to ease the complexity of MediaElemnt in the beginning. So that we follow the nsIChannel design and create the RtspChannel code. It's jobs are passed the URL and Rtsp mine type in media element. I'll make a discussion with Bechen and try to integrate the RtspChannel and RtspController together on next week. May I also have your comments about interfaces defined in nsIStreamingProtocolController.idl(nsIStreamingMediaController.idl ??)

For keeping it generic, keeping using RtspChannel is more reasonable. Current implementation of RtspChannel is too complex, there is too much unused code. I suggest to remove all IPC code of RtspChannel to make it simple and left it as a dummy placeholder for passing URI.

It sounds reasonable for me, I can post a new patch to remove all IPC code soon if Jason and Steve agree with that.

I still think the right approach here is what I said in comment 61: -- I think we should special-case rtsp here [in HTMLMediaElement::UpdatePreloadAction()], and not use NS_NewChannel. I'm guessing we should pass the rtsp URI to the MediaResourceRTSP, and have it create a RtspController class (via do_CreateInstance, like we do for WebSocketChannels here): http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#780 -- If that approach really is too hard somehow, I'm willing to look at a patch that just uses RtspChannel as a placeholder to pass the URI. It's definitely a kludge to implement a whole nsIChannel class just to hand off a URI to another class.

Comment on attachment 727570 [details] [diff] [review] WIP 2, refactor RTSP channel and RTSP handler Review of attachment 727570 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/rtsp/RTSPChannel.h @@ +32,5 @@ > + > +namespace mozilla { > +namespace net { > + > +class RTSPChannel : public BaseRTSPChannel RTSPChannel can derive nsBaseChannel, like what nsDataChannel did, instead of BaseRTSPChannel that can be removed. By inheriting nsBaseChannel, you only implement it with a few lines of code.

> inheriting nsBaseChannel Yes, it could probably save some lines of code if we have to use NS_NewChannel. But I still think comment 69 is correct, i.e. avoid NS_NewChannel.

To be clear--if it takes more than an hour or two to implement my suggestion in comment 69, I'm fine with doing the fake nsIChannel approach. I don't want to block progress here. But give it a quick try!

I agree with Jason here: it's only a few lines of code to implement it, but the class/object itself has a lot of redundancy. One compromise might be for RtspChannel to own/create RtspController, and return it to RtspMediaResource. If it takes too long to merge the classes or have RtspChannel own/Create RtspController, then please put a comment/several comments in RtspChannel. For future code changes, it will be useful to explain that RtspChannel is primarily there for abstract channel and MediaResource creation in HTMLMediaElement, and for passing the uri to RtspController.

Support the live stream. RTSPTrackBuffer bug fix.

Support live stream. Separate the RTSPOmxDecoder and RTSPOmxReader into other files.

1. Switch the capitalization from RTSP to Rtsp

1. Switch the capitalization from RTSP to Rtsp 2. Add MOZ_RTSP to Makefile.in and moz.build

1. Add MOZ_RTSP to moz.build 2. Remove IPC for RtspChannel

1. Add RtspChannel for abstract channel and MediaResource creation in HTMLMediaElement, and for passing the uri to RtspController. 2. Switch the capitalization from RTSP to Rtsp 3. Remove IPC

This is copied from Android's libstagefright and should be the same with attach 727572(previous one). To be safe, attach it again.

1. Add MOZ_RTSP to toolkit/library/Makefile.in

1. Switch the capitalization from RTSP to Rtsp

Comment on attachment 746286 [details] [diff] [review] 1. Patch v03 - Add MOZ_RTSP compiler flag Review of attachment 746286 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4610,5 @@ > MOZ_WEBGL=1 > MOZ_PDF_PRINTING=1 > MOZ_TOUCH=1 > + MOZ_RTSP=1 > + AC_SUBST(MOZ_RTSP) AC_SUBSTs don't actually matter where you put them in the file, they get SUBSTed just the same. I think it'd be nicer to put this in a different place, and also do the usual if test -n "$MOZ_RTSP"; then AC_DEFINE(MOZ_RTSP) fi instead of sticking it all in the cairo-gonk block.

Address the review comment.

Comment on attachment 747763 [details] [diff] [review] 1. Patch v04 - Add MOZ_RTSP compiler flag Review of attachment 747763 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 746298 [details] [diff] [review] 5. Part1 Patch v04 - Implement RtspChannel and RtspProtocolHandler Review of attachment 746298 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4253,5 @@ > MOZ_DISABLE_CRYPTOLEGACY= > NSS_DISABLE_DBM= > NECKO_WIFI=1 > NECKO_COOKIES=1 > +NECKO_PROTOCOLS_DEFAULT="about data file ftp http res viewsource websocket wyciwyg device rtsp" Can you do this so NECKO_PROTOCOLS_DEFAULT contains rtsp only for B2G? Or will rtsp be filtered out somewhere else?

> > +NECKO_PROTOCOLS_DEFAULT="about data file ftp http res viewsource websocket wyciwyg device rtsp" > > Can you do this so NECKO_PROTOCOLS_DEFAULT contains rtsp only for B2G? > Or will rtsp be filtered out somewhere else? Hi Steve, thanks for your comment. I am going to post a new patch for that, and also fix the try server error.

Comment on attachment 746291 [details] [diff] [review] 2. Patch v07 - RtspMediaResource and MediaElement Review of attachment 746291 [details] [diff] [review]: ----------------------------------------------------------------- I'm about halfway through MediaResource.cpp. I'll come back to the rest tomorrow. ::: content/html/content/src/HTMLMediaElement.cpp @@ +336,5 @@ > element && > + NS_SUCCEEDED(rv = element->InitializeDecoderForChannel(channel, getter_AddRefs(mNextListener)))) { > + if (mNextListener) { > + rv = mNextListener->OnStartRequest(aRequest, aContext); > + } Is this change needed? @@ +2405,5 @@ > > + if (DecoderTraits::DecoderWaitsForOnConnected(mimeType)) { > + // We postpone the |FinishDecoderSetup| function call until we get > + // |OnConnected| signal from MediaStreamController > + // which is held by RtspMediaResource. Please reformat the text in this comment so that up to 80 characters are used per line. It's a small thing, but it's good for keeping clean code. @@ +2408,5 @@ > + // |OnConnected| signal from MediaStreamController > + // which is held by RtspMediaResource. > + decoder->SetResource(resource); > + mDecoder = decoder; > + *aListener = nullptr; Null check for aListener before dereferencing. ::: content/media/MediaResource.cpp @@ +1263,5 @@ > mIgnoreResume = false; > } > } > > +/* class RtspTrackBuffer: a ring buffer implementation. ... for audio/video track data? @@ +1274,5 @@ > +#define BUFFER_SLOT_DEFAULT_SIZE 1024 > +#define BUFFER_SLOT_MAX_SIZE 8192 > +#define BUFFER_SLOT_INVALID -1 > +#define BUFFER_SLOT_EMPTY 0 > +class RtspTrackBuffer Empty line between #defs and class. @@ +1278,5 @@ > +class RtspTrackBuffer > +{ > +public: > + RtspTrackBuffer(const char *monitor, int32_t aTrackIdx, uint32_t aSlotSize) > + : mMonitor(monitor), Something Jason suggested to me when I started: Put the colon on the next line (like you did), and then the comma on the next line. Something like... : mMonitor , mTrackIdx etc. The reason is that if you need to add, change or remove a variable, only one line is changed here. This is good for future reviews and the revision history. And it's easy to change it now before committing. @@ +1307,5 @@ > + mIsStarted = false; > + } > + // Read the data from mSrcBuffer[mConsumerIdx*mSlotSize] > + nsresult ReadBuffer(uint8_t* aBuffer, uint32_t aBufferSize, > + uint32_t* aBytes, uint64_t *aTime, uint32_t *aFrameSize); ... into aBuffer, right? @@ +1309,5 @@ > + // Read the data from mSrcBuffer[mConsumerIdx*mSlotSize] > + nsresult ReadBuffer(uint8_t* aBuffer, uint32_t aBufferSize, > + uint32_t* aBytes, uint64_t *aTime, uint32_t *aFrameSize); > + // Write the data into mSrcBuffer[mProducerIdx*mSlotSize] > + void WriteBuffer(const char *data, uint32_t aCount, uint64_t aTime, uint32_t aFrameType); Nit: Please use similar/consistent names for the params in ReadBuffer and WriteBuffer, something like: ReadBuffer(uint8_t* aToBuffer, uint32_t aToBufferSize, uint32_t* aReadCount, ... WriteBuffer(const uint8_t* aFromBuffer, uint32_t aWriteCount... @@ +1332,5 @@ > + } > + > + // The FrameType is sync to nsIStreamingProtocolController.h > + //#define MEDIASTREAM_FRAMETYPE_NORMAL 0x00000001 > + //#define MEDIASTREAM_FRAMETYPE_DISCONTINUNITY 0x00000002 Don't add these defs here, even in comment. The reference to nsIStreamingController.h is enough - it should be the authoritative and only source for these values. @@ +1369,5 @@ > + > +/* The value in mSrcBufferDataLength[index] represents: > + * -1(BUFFER_SLOT_SKIP): The index of slot data is invalid, mConsumerIdx should go forward. > + * 0(BUFFER_SLOT_EMPTY): The index slot is empty. mConsumerIdx should wait here. > + * positive value: The index slot contains valid data and the value is data size. I think this is better beside mSrcBufferDataLength in the class definition. @@ +1372,5 @@ > + * 0(BUFFER_SLOT_EMPTY): The index slot is empty. mConsumerIdx should wait here. > + * positive value: The index slot contains valid data and the value is data size. > + * > + * When aBufferSize is smaller than aFrameSize, the reader should realloc the > + * aBuffer and read again. This is better place in the class definition beside the function prototype. Explain that the function will return if aBufferSize < aFrameSize, and the calling function MUST realloc more space if it wishes to read the data. @@ +1374,5 @@ > + * > + * When aBufferSize is smaller than aFrameSize, the reader should realloc the > + * aBuffer and read again. > + * > + * After reading the data, we clear the mSrcBufferDataLength. You clear the whole array or just the current index? @@ +1378,5 @@ > + * After reading the data, we clear the mSrcBufferDataLength. > + * */ > +nsresult RtspTrackBuffer::ReadBuffer(uint8_t* aBuffer, uint32_t aBufferSize, > + uint32_t* aBytes, uint64_t *aTime, > + uint32_t *aFrameSize) { Opening brace on new line for functions defined outside a class. (See Mozilla code style guide for more info). @@ +1382,5 @@ > + uint32_t *aFrameSize) { > + MonitorAutoLock monitor(mMonitor); > + *aBytes = 0; > + *aTime = 0; > + *aFrameSize = 0; Null check for these vars. @@ +1388,5 @@ > + RTSPMLOG("NS_ERROR_OUT_OF_MEMORY"); > + return NS_ERROR_OUT_OF_MEMORY; > + } > + RTSPMLOG("ReadBuffer mTrackIdx %d mProducerIdx %d mConsumerIdx %d mSrcBufferDataLength[mConsumerIdx] %d", > + mTrackIdx, mProducerIdx, mConsumerIdx, mSrcBufferDataLength[mConsumerIdx]); Clean up the formatting of this LOG: 80 chars max per line. @@ +1394,5 @@ > + if (mSrcBufferDataLength[mConsumerIdx] > 0) { > + // check the buffer space is enough for data copy > + if (aBufferSize < mSrcBufferDataLength[mConsumerIdx]) { > + *aFrameSize = mSrcBufferDataLength[mConsumerIdx]; > + break; Looks like this ends up returning. You should consider if you want it to return an error code instead of breaking and falling through to "return NS_OK". This can be an indication to the calling function that it should check if aBufferSize < aFrameSize. @@ +1419,5 @@ > + // no data, and disconnected. > + if (!mIsStarted) { > + return NS_ERROR_FAILURE; > + } > + // no data, the decode thread is blocked here. ... until? @@ +1436,5 @@ > + * To ensure the decoder will get the "oldest" data in RtspTrackBuffer. > + * > + * If the incoming data is larger than one slot size (isMultipleSlots), we do > + * |mSrcBufferDataLength[] = BUFFER_SLOT_INVALID;| for other slots except the > + * first slot. In order to notice the reader some slots are invalid. "Invalid" might be the wrong word to use here - it suggest that something is wrong with the buffer slot. "Unavailable" or "Used" or "Extension" might be better. @@ +1454,5 @@ > + RTSPMLOG("mTotalBufferSize < aCount, incoming data is too large"); > + return; > + } > + > + if (aFrameType & MEDIASTREAM_FRAMETYPE_DISCONTINUNITY) { There's one too many 'n's in DISCONTINU_ITY :) @@ +1464,5 @@ > + if (mFrameType & MEDIASTREAM_FRAMETYPE_DISCONTINUNITY) { > + // the MEDIASTREAM_FRAMETYPE_DISCONTINUNITY bit is set, drop the frame > + // until we receive MEDIASTREAM_FRAMETYPE_DISCONTINUNITY. > + return; > + } This and the previous condition statements are very similar - please add a comment before each one: // Checking incoming frame type. ... // Checking current buffer frame type. ... Or whatever is appropriate. ::: content/media/MediaResource.h @@ +376,5 @@ > // nsIChannel when the MediaResource is created. Safe to call from > // any thread. > virtual const nsCString& GetContentType() const = 0; > + > + //Returns the nsIStreamingProtocolController in the MediaResource.(Rtsp) Space before "Returns". @@ +384,5 @@ > + // Read data from track. > + // aBuffer, aBufferSize: buffer pointer and size > + // aBytes: output actual read bytes. > + // aTime: outpu time stamp > + // aFrameSize: actual data size in track Thanks for adding extra info about the parameters. Please indent them a little from the general comment. @@ +677,5 @@ > + * If there are one audio and one video stream, the RtspMediaResource will create > + * two RtspTrackBuffer for their data and also provides ReadTrack functions for > + * decoder to get the data. > + * > + * Rtsp start flow: Thanks for this explanation. @@ +700,5 @@ > + * > + * The decoder thread call the ReadTrack(uint32_t aTrackIdx ...) to get the > + * data in RtspTrackBuffer. The decoder thread will be blocked if there is > + * no data in RtspTrackBuffer. > + * */ Can you add some comments about seeking and playback control? @@ +744,5 @@ > + { > + return NS_ERROR_FAILURE; > + } > + > + // These methods are called off the main thread. Are they called on a specific thread or just any other thread? @@ +755,5 @@ > + virtual void StartSeekingForMetadata() {} > + virtual void EndSeekingForMetadata() {} > + virtual int64_t Tell() {/* TODO:??*/return 0;} > + > + // Any thread Do you need a mutex or monitor for these functions? @@ +783,5 @@ > + > + void Revoke() { mResource = nullptr; } > + // We don't use the |OnDataAvailable| in NS_DECL_NSISTREAMLISTENER, > + // instead we use the |OnDataAvailable| NS_DECL_NSIMEDIASTREAMLISTENER > + // for Rtsp. Thanks for the clarification - good for future readers! @@ +793,5 @@ > +protected: > + // Main thread access only > + nsRefPtr<Listener> mListener; > + // These are called on the main thread by Listener. > + nsresult OnMediaDataAvailable(uint8_t index, const char *data, uint32_t length, nsIStreamingProtocolMetaData *meta); Please format this prototype so that 80 chars per line max are used.

Comment on attachment 746291 [details] [diff] [review] 2. Patch v07 - RtspMediaResource and MediaElement Review of attachment 746291 [details] [diff] [review]: ----------------------------------------------------------------- Good work. I like how this is coming along, and I can definitely see how the data is piped through RtspMediaResource to the decoder. I made several comments on formatting and indentation - can you check the mozilla style guide and make sure you're following the guidelines there. Keep up the good work! ::: content/media/MediaResource.cpp @@ +1289,5 @@ > + if(!mSrcBuffer) { > + RTSPMLOG("malloc fail, NS_ERROR_OUT_OF_MEMORY"); > + } else { > + mTotalBufferSize = BUFFER_SLOT_NUM * mSlotSize; > + } Why not use 'new' here? It's infallible so you don't need the null check. Also, you should check that aSlotSize > 0 before attempting to create the array. mSrcBuffer = new uint8_t[BUFFER_SLOT_NUM * mSlotSize]; (See https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation for more on infallible allocation). @@ +1327,5 @@ > + // If we call reset() first, the queue may still has some "garbage" frame > + // from another thread's |OnDataAvailable| before |SetFrameType|. > + void ResetWithFrameType(uint32_t aFrameType) { > + SetFrameType(aFrameType); > + Reset(); Should these be locked together? In this code, the lock is released between the function calls. You might want to consider ReentrantMonitor. But, if you don't want the added overhead, you could create public Reset and SetFrameType functions, which lock the monitor, and internal versions which verify that the monitor has been entered (mMonitor.AssertCurrentThreadOwns()). @@ +1390,5 @@ > + } > + RTSPMLOG("ReadBuffer mTrackIdx %d mProducerIdx %d mConsumerIdx %d mSrcBufferDataLength[mConsumerIdx] %d", > + mTrackIdx, mProducerIdx, mConsumerIdx, mSrcBufferDataLength[mConsumerIdx]); > + while (1) { > + if (mSrcBufferDataLength[mConsumerIdx] > 0) { Can you rename this mBufferSlotDataLength - it's clearer that the index refers to a slot this way, rather than a whole new buffer. @@ +1402,5 @@ > + slots = (mSrcBufferDataLength[mConsumerIdx] / mSlotSize) + 1; > + // we have data, copy to aBuffer > + memcpy(aBuffer, > + (void *)(&mSrcBuffer[mSlotSize * mConsumerIdx]), > + mSrcBufferDataLength[mConsumerIdx]); If this is a ring buffer, then what if the data wraps around at the end of the array? @@ +1490,5 @@ > + for (i = mProducerIdx; i < BUFFER_SLOT_NUM; ++i) { > + mSrcBufferDataLength[i] = BUFFER_SLOT_INVALID; > + } > + // we overwrite a slot that the decode thread doesn't retrieve > + // So the mConsumerIdx returns to head too and move forward to the oldest slot So, the producer does not have to stop writing data because it is waiting for the consumer to catch up. Instead, the data is implicitly discarded because the consumer is automatically moved to the oldest data. Nice. Out of interest, how does this work for the decoder if it misses an I-Frame? Or does RTSP have a way to work around that? @@ +1503,5 @@ > + } > + mProducerIdx = 0; > + } > + > + memcpy(&(mSrcBuffer[mSlotSize * mProducerIdx]), data, aCount); Again, this doesn't seem to wrap around the array. @@ +1611,5 @@ > + aTime, aFrameSize); > +} > + > +nsresult > +RtspMediaResource::OnMediaDataAvailable(uint8_t index, const char *data, Please rename index to aTrackIdx. @@ +1624,5 @@ > + > +nsresult > +RtspMediaResource::OnConnected(uint8_t index, int32_t fd, > + nsIStreamingProtocolMetaData *meta) { > + if (!mIsOnConnected) { Please rename to mIsConnected. @@ +1637,5 @@ > + trackMeta->GetDuration(&duration); > + // TODO: how to return NS_ERROR_OUT_OF_MEMORY to the HTMLMediaElement > + // when we create RtspTrackBuffer fail? > + // Since we cut the code flow on HTMLMediaElement::InitializeDecoderForChannel > + // for Rtsp. You can notify using NetworkError or DecodeError. @@ +1659,5 @@ > + mDecoder->SetTransportSeekable(seekable); > + } else { > + // live stream > + // enable realtime mode and create a realtime MediaDecoderStateMachine > + // Preferences::SetBool("media.realtime_decoder.enabled", true); Why is this comment about Preferences here? Do you intend to set it true, or do you mean to say it must be true for real-time streaming to proceed? @@ +1688,5 @@ > + mTrackBuffer[i]->Stop(); > + mTrackBuffer[i]->Reset(); > + } > + return NS_OK; > +} Do you need to set mIsOnConnected to false here? When is mTrackBuffer deleted? @@ +1707,5 @@ > + > + if (mChannel) { > + if (aCloseImmediately) { > + // TODO: workaround here, we dont close the channel and listener now. > + //Close(); Out of interest, why do you not close the channel here? @@ +1749,5 @@ > + return NS_OK; > +} > + > +already_AddRefed<nsIPrincipal> RtspMediaResource::GetCurrentPrincipal() { > + // TODO: Does this function work?? Jason would know better than me - is the question whether secMan can get the info from RtspChannel or not? @@ +1767,5 @@ > + RtspMediaResource* resource = new RtspMediaResource(aDecoder, > + nullptr, > + mURI, > + GetContentType()); > + // TODO:? For a first iteration, you could just create copies of all the important member variables. Later you can try to optimize with multiple RtspMediaResources using the same data source ... if that's possible. @@ +2141,5 @@ > +static bool IsRtspURI(nsIURI* aUri) { > + bool ret = false; > + aUri->SchemeIs("rtsp", &ret); > + return ret; > +} Put this with the definition for IsBlobURI. You should ask someone who has worked on that file to provide feedback and review the changes.

Comment on attachment 746291 [details] [diff] [review] 2. Patch v07 - RtspMediaResource and MediaElement Review of attachment 746291 [details] [diff] [review]: ----------------------------------------------------------------- Setting r- to remove this from my review queue. ::: content/media/MediaResource.cpp @@ +1289,5 @@ > + if(!mSrcBuffer) { > + RTSPMLOG("malloc fail, NS_ERROR_OUT_OF_MEMORY"); > + } else { > + mTotalBufferSize = BUFFER_SLOT_NUM * mSlotSize; > + } Additional comment on this allocation: You can declare mSrcBuffer to be an nsAutoArrayPtr and then you can remove the delete in the destructor. See nsHtml5StreamParser::OnDataAvailable for an example of using nsAutoArrayPtr to create a byte array for memcpy.

rebase and address the try server errors

rebase and address try server errors.

rebase and address try server errors

1. streaming protocol service and controller interface. 2. address trying server errors.

rebase and address trying server error.

(In reply to Steve Workman [:sworkman] from comment #90) > > @@ +1688,5 @@ > > + mTrackBuffer[i]->Stop(); > > + mTrackBuffer[i]->Reset(); > > + } > > + return NS_OK; > > +} > > Do you need to set mIsOnConnected to false here? When is mTrackBuffer > deleted? For now it will be destroyed at ~RtspMediaResource . Current flow: after AsyncOpen(our new listener) The listener will trigger the callback function OnConnected and OnDisconnected. For suspend and resume implementation (not finish), we consider to cut off the connection between Rtsp server and DUT. And this implementation will affect the OnConnected and OnDisconnected control flow. Such as the suspend action should trigger OnDisconnected ? Do you have any suggestion to do this?

(In reply to Steve Workman [:sworkman] from comment #90) > Comment on attachment 746291 [details] [diff] [review] > > @@ +1327,5 @@ > > + // If we call reset() first, the queue may still has some "garbage" frame > > + // from another thread's |OnDataAvailable| before |SetFrameType|. > > + void ResetWithFrameType(uint32_t aFrameType) { > > + SetFrameType(aFrameType); > > + Reset(); > > Should these be locked together? In this code, the lock is released between > the function calls. You might want to consider ReentrantMonitor. But, if you > don't want the added overhead, you could create public Reset and > SetFrameType functions, which lock the monitor, and internal versions which > verify that the monitor has been entered > (mMonitor.AssertCurrentThreadOwns()). > The code flow is ok if the lock is released between these function calls. > > @@ +1402,5 @@ > > + slots = (mSrcBufferDataLength[mConsumerIdx] / mSlotSize) + 1; > > + // we have data, copy to aBuffer > > + memcpy(aBuffer, > > + (void *)(&mSrcBuffer[mSlotSize * mConsumerIdx]), > > + mSrcBufferDataLength[mConsumerIdx]); > > If this is a ring buffer, then what if the data wraps around at the end of > the array? The data would not wrap around the end of array, I add protection |isReturnToHead| in WriteBuffer. ex: if there is 1 slot left and the incoming data needs 2 slots, the WriteBuffer will put the data to the begging of array. Maybe I should add some check before memcpy. > @@ +1490,5 @@ > > + for (i = mProducerIdx; i < BUFFER_SLOT_NUM; ++i) { > > + mSrcBufferDataLength[i] = BUFFER_SLOT_INVALID; > > + } > > + // we overwrite a slot that the decode thread doesn't retrieve > > + // So the mConsumerIdx returns to head too and move forward to the oldest slot > > So, the producer does not have to stop writing data because it is waiting > for the consumer to catch up. Instead, the data is implicitly discarded > because the consumer is automatically moved to the oldest data. > Nice. > Out of interest, how does this work for the decoder if it misses an I-Frame? > Or does RTSP have a way to work around that? Currently we have only one discard policy. Discard the oldest one frame. And I don't know what happened if the I-Frame is missed, it depends the decoder. I guess the decoder still work properly but some block of the video are wrong with color. If we need more discard policy (drop p/b frame..), we will need some "frame sniff" for every codec type to detect the frame type. It's better to implement them at libstagefright/rtsp/XXXAssembler.cpp. And also we need a policy to turn on/off the discard action. (in RtspTrackBuffer, we can observe the "overwrite" count. or the omx decoder can return some status such as FPS) > > @@ +1659,5 @@ > > + mDecoder->SetTransportSeekable(seekable); > > + } else { > > + // live stream > > + // enable realtime mode and create a realtime MediaDecoderStateMachine > > + // Preferences::SetBool("media.realtime_decoder.enabled", true); > > Why is this comment about Preferences here? Do you intend to set it true, or > do you mean to say it must be true for real-time streaming to proceed? > The preference "media.realtime_decoder.enabled" must be true for the live stream. Code in MediaDecoderStateMachine constructor will overwrite |mRealTime| it if the preference is false. Or the playback won't succeed, it will show "black screen" due to the MediaDecoderStateMachine expect to get the frame with timestamp, but the live stream content doesn't contain timestamp. Should I check the preference and return some error? Or just leave it if the preference is false? > > @@ +1707,5 @@ > > + > > + if (mChannel) { > > + if (aCloseImmediately) { > > + // TODO: workaround here, we dont close the channel and listener now. > > + //Close(); > > Out of interest, why do you not close the channel here? > The channel is dummy now, I will consider to remove the code about it.

So are all the patches here (1-9) meant to be applied in order, or is it just parts 0-4 (patch 4-9)? I don't see any tests--is there a way to test this manually?

Yes, these patches should be applied in order, patches(1~3) and patches(4~9). We don't have the test cases right now. We use the simple gaia app(RTSP test gaia app) and connect to mobile youtube directly. http://m.youtube.com/. There are a lot of Rtsp streaming contents there. Here is an example. rtsp://v2.cache4.c.youtube.com/CjYLENy73wIaLQltdR599uAuqBMYDSANFEIJbXYtZ29vZ2xlSARSBXdhdGNoYM7uhMP11rrfTww=/0/0/0/video.3gp We are also trying to setup the local Rtsp server, we have tried Darwin and Live555Media server. Our plan is to run the local server in emulator, and check the content of media sample.

Comment on attachment 746292 [details] [diff] [review] 3. Patch v07 - RtspDecoder Review of attachment 746292 [details] [diff] [review]: ----------------------------------------------------------------- Good work - looks like this patch has got to the cleanup stage rather than working on functionality. Almost there. Also, can you consider multi-threading in the comments and by adding NS_ASSERTION(<thread>...) to each function. The media engine is complex with respect to threads, so we want to make sure each function is running on the right thread from the start. ::: content/media/DecoderTraits.cpp @@ +251,5 @@ > +} > +#endif > + > +/* static */ > +bool DecoderTraits::DecoderWaitsForOnConnected(const nsACString& aType) { Rename aType to aMimeType to be 100% clear. ::: content/media/MediaDecoder.cpp @@ +1155,5 @@ > + // except seek. > + // For PLAY_STATE_SEEKING state, > + // mDecoderStateMachine->Seek-> RtspOmxReader::Seek-> resource->SeekTime-> > + // controller->Seek(). > + nsIStreamingProtocolController* controller = mResource->GetMediaStreamController(); I'd rather see ChangeState overridden in RtspOmxDecoder. If another streaming decoder comes later, then we can make this change. But for now, the code is too specific to have in the base class. However, I realize that the changes are complex. So, I suggest adding a private sub function for ApplyStateToStateMachine. -- Replace the 'if' block with 'ApplyStateToStateMachine(aState);'. -- Use the 'if' block as the body for the function. -- Add a check to make sure we're in the monitor. -- Add a default version in MediaDecoder; make it virtual. -- Add an overriding version in RtspOmxDecoder. This way, we still get to have a single ChangeState, but you don't need to include nsIStreamingProtocolController.h or even mention it here. @@ +1670,5 @@ > +bool > +MediaDecoder::IsRtspEnabled() > +{ > + //Currently the Rtsp decoded by omx. > + return Preferences::GetBool("media.omx.enabled", false); "RTSP streams are decoded using OMX decoder; verify it is enabled." ::: content/media/MediaDecoder.h @@ +300,5 @@ > { > return mResource; > } > > + void SetResource(MediaResource* resource) MOZ_FINAL Whitespace. I don't think you need the MOZ_FINAL here. This function is not virtual and not overriding anything, right? ::: content/media/omx/Makefile.in @@ +17,5 @@ > MediaOmxReader.cpp \ > OmxDecoder.cpp \ > $(NULL) > > +ifdef MOZ_RTSP Whitespace. ::: content/media/omx/MediaOmxReader.cpp @@ +64,5 @@ > NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread."); > > *aTags = nullptr; > > + InitDecoder(); Just to be extra clear here, can you add a comment to say "Initializing internal OMX Decoder." ::: content/media/omx/MediaOmxReader.h @@ +32,5 @@ > int64_t mAudioSeekTimeUs; > int32_t mSkipCount; > + > +protected: > + // mOmxDecoder need to be in protected segment. Please extend this comment to explain why it should be protected. Otherwise, just remove the comment - if it's in the protected section, it's implicit that it needs to be protected. @@ +36,5 @@ > + // mOmxDecoder need to be in protected segment. > + android::sp<android::OmxDecoder> mOmxDecoder; > + > + // To be overwrite for customize extractor. > + virtual void InitDecoder(); I'm not sure if you need this comment. Better to describe it generally: "Called by <x> during <y>. Initializes decoder including setting up custom extractor." This is a data extractor, right? What does it extract from. Also, can you rename this to InitOmxDecoder, please? Currently, it's ambiguous whether it refers to the android OmxDecoder or RtspOmxDecoder. (In general, this omx naming stuff is hard to keep track of, but that's not your fault and there's not much you can do about it :) ) ::: content/media/omx/OmxDecoder.cpp @@ +207,5 @@ > return false; > } > > + // Note that in the |MediaOmxReader::InitDecoder()| we call > + // |mDecoder->GetResource()->SetReadMode(MediaCacheStream::MODE_METADATA)| Why? @@ +258,5 @@ > !(videoSource->getFormat()->findInt32(kKeyWidth, &width) && > videoSource->getFormat()->findInt32(kKeyHeight, &height) && > width * height <= maxWidth * maxHeight)) { > + LOG(PR_LOG_DEBUG,"Failed to get video size, or it was too large for HW decoder (<w=%d, h=%d> but <maxW=%d, maxH=%d>)", > + width, height, maxWidth, maxHeight); Since you're already changing this, please reduce the length of the line to 80 chars max. ::: content/media/omx/OmxDecoder.h @@ +140,5 @@ > public: > OmxDecoder(MediaResource *aResource, AbstractMediaDecoder *aDecoder); > ~OmxDecoder(); > > + bool Init(sp<MediaExtractor>& extractor); Can you add a comment about extractor, its role and who will supply it? ::: content/media/omx/RtspOmxDecoder.cpp @@ +10,5 @@ > + > +namespace mozilla { > + > +RtspOmxDecoder::RtspOmxDecoder() : > + MediaDecoder() This is small enough to put in the class definition. Same with destructor. @@ +26,5 @@ > + bool realTime = false; > + if (mResource) { > + realTime = mResource->IsRealTime(); > + } > + return new MediaOmxStateMachine(this, new RtspOmxReader(this), realTime); In another comment we discussed the realtime pref. If MediaOmxStateMachine checks this pref and it is disabled, but realTime is true, what happens? ::: content/media/omx/RtspOmxDecoder.h @@ +10,5 @@ > +#include "MediaDecoder.h" > + > +namespace mozilla { > + > +class RtspOmxDecoder : public MediaDecoder RtspOmxReader inherits from MediaOmxReader - should RtspOmxDecoder inherit from MediaOmxDecoder? If not, please comment on that difference here and in RtspOmxReader.h. ::: content/media/omx/RtspOmxReader.cpp @@ +27,5 @@ > +namespace mozilla { > + > +/* class RtspMediaSource : implements MediaSource for OMX. > + * When the read() be called by OMX decoder, it reads data from mResource > + * and construct a MediaBuffer for output. "When read() is called by OMX decoder, ..." Output to where, for what? @@ +41,5 @@ > + mFormat(aMeta), > + mGroup(nullptr), > + mBuffer(nullptr), > + mFrameMaxSize(aFrameMaxSize) > + {}; Add MOZ_CTOR(RtspMediaResource) to this constructor to monitor memory usage and help detect leaks. Similarly, add MOZ_DTOR(RtspMediaResource) to the destructor. You can see examples of this in the media code. Do this for all new classes. Change member construction to: ... const sp<MetaData>& aMeta) : mResource(aMediaResource) , mTrackIdx(aTrackIdx) , mFormat(aMeta) etc. This is better for revision history later as it allows for one line changes. Please review all constructors for this. @@ +51,5 @@ > + // TODO: do we need lock? > + return mFormat; > + }; > + virtual status_t read(MediaBuffer **buffer, > + const ReadOptions *options = nullptr); Please add "MOZ_FINAL MOZ_OVERRIDE" to each of these functions and remove the keyword virtual - it is only needed for the base class. Please review all virtual and overridden functions for this. (Note: Here is a good explanation of the macros: http://whereswalden.com/2011/11/26/introducing-moz_final-prevent-inheriting-from-a-class-or-prevent-overriding-a-virtual-function/) @@ +61,5 @@ > + // output for omx > + // mGroup owns the mBuffer. mFrameMaxSize is the mBuffer size. > + MediaBufferGroup *mGroup; > + MediaBuffer *mBuffer; > + uint32_t mFrameMaxSize; Please add comments to each of these variables. Something like: // |mResource| gets data from the network which is written into |mBuffer|. // |mBuffer| holds data which is read by the OMX decoder. @@ +67,5 @@ > + > +RtspMediaSource::~RtspMediaSource() { > + if (mGroup) { > + delete mGroup; > + mGroup = nullptr; delete is null safe. You shouldn't need the 'if (mGroup)'. Or better, can you use an nsAutoPtr here to avoid 'delete' and '= nullptr', please? @@ +75,5 @@ > +status_t RtspMediaSource::start(MetaData *params) { > + mGroup = new MediaBufferGroup(); > + MediaBuffer *p = new MediaBuffer(mFrameMaxSize); > + if (p && mGroup) { > + mGroup->add_buffer(p); '{' on newline for out of class functions. Can start be called twice? If so, you need to consider what happens to the old mGroup. Does RtspMediaSource relinquish ownership of the MediaBuffer here? Add a comment to clarify this, please. @@ +89,5 @@ > + mBuffer->release(); > + mBuffer = nullptr; > + } > + delete mGroup; > + mGroup = nullptr; If you use nsAutoPtr, just set to nullptr here. @@ +95,5 @@ > + return OK; > +} > + > +status_t RtspMediaSource::read(MediaBuffer **out, const ReadOptions *options) { > + *out = nullptr; nullptr check for out and options please. @@ +112,5 @@ > + if (err != OK) { > + return err; > + } > + rv = mResource->ReadTrack((uint8_t *)mBuffer->data(), mFrameMaxSize, > + mTrackIdx, &read, &time, &actualFrameSize); Please check rv before continuing. This is reading one frame, right? If so, if might be better to change the function name to ReadFrame or ReadFrameFromTrack. If it reads more than one frame, then add a comment to explain how much it might read. @@ +137,5 @@ > + } > + return ERROR_END_OF_STREAM; > +} > + > +class RtspExtractor: public MediaExtractor { Add a comment to explain what this class does. @@ +141,5 @@ > +class RtspExtractor: public MediaExtractor { > +public: > + virtual size_t countTracks(); > + virtual sp<MediaSource> getTrack(size_t index); > + virtual sp<MetaData> getTrackMetaData(size_t index, uint32_t flag = 0); Remove virtual, add 'MOZ_FINAL MOZ_OVERRIDE'. @@ +147,5 @@ > + RtspExtractor(MediaResource *resource); > + > +private: > + MediaResource *mResource; > + nsRefPtr<nsIStreamingProtocolController> mController; Add comments to explain the roles of these two members. @@ +179,5 @@ > + return nullptr; > + > + sp<MetaData> meta = new MetaData(); > + nsCOMPtr<nsIStreamingProtocolMetaData> nsMeta; > + mController->GetTrackMetaData(index, getter_AddRefs(nsMeta)); nsMeta looks like a class name: maybe rtspMetadata and omxMetadata? This is clearer re which components the metadata came from. It is also very clear at the end of the function which type of data you are returning. Please check nsMeta (rtspMetadata) for null; meta (omxMetadata) should be fine as 'new' is infallible. @@ +188,5 @@ > + meta->setCString(kKeyMIMEType, mime.get()); > + uint32_t i; > + nsMeta->GetWidth(&i); > + meta->setInt32(kKeyWidth, i); > + nsMeta->GetHeight(&i); Is it guaranteed that 'i' will be reset here? What if GetHeight fails and 'i' is returned with the same value as GetWidth? Checking return values from GetWidth etc. seems vary cumbersome here, but manually resetting 'i' to 0 (or some such value) each time may help with debugging problems later. Also, i and j are normally used with nested for loops. Maybe temp32 and temp64 are better names. @@ +198,5 @@ > + uint64_t j; > + nsMeta->GetDuration(&j); > + meta->setInt64(kKeyDuration, j); > + > + nsCString a; 'a' is not a good var name: tempCString is better. @@ +215,5 @@ > +RtspOmxReader::RtspOmxReader(AbstractMediaDecoder* aDecoder) > + : MediaOmxReader(aDecoder) { > +} > + > +void RtspOmxReader::InitDecoder() MOZ_OVERRIDE { MOZ_OVERRIDE is only needed in the class definition. @@ +230,5 @@ > + if (resource) { > + resource->SeekTime(aTime); > + } > + > + return MediaOmxReader::Seek(aTime, aStartTime, aEndTime, aCurrentTime); Please explain why you call mResource->SeekTime before calling MediaOmxReader::Seek. Without a comment it looks like you're doing two seeks which might conflict. Explain how RTSP seeking works with DecodeToTarget. Also, consider changing this - do you really need all that functionality in DecodeToTarget? Could you add similar code here that knows that it will receive the correct frame next? If not, then explain why you must use DecodeToTarget (via Seek). ::: content/media/omx/RtspOmxReader.h @@ +17,5 @@ > +} > + > +class AbstractMediaDecoder; > + > +// RtspOmxReader is a subclass of MediaOmxReader. Remove this comment unless there is an important reason why MediaOmxReader is the base class. The inheritance is clear enough in the code. @@ +21,5 @@ > +// RtspOmxReader is a subclass of MediaOmxReader. > +class RtspOmxReader : public MediaOmxReader > +{ > +protected: > + void InitDecoder() MOZ_OVERRIDE; MOZ_FINAL MOZ_OVERRIDE Same with other functions in this class, unless you plan to override them in subclasses. Explain why you are overriding the function. @@ +29,5 @@ > + // Override the seek() because Rtsp uses time-based seek function. > + nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, > + int64_t aCurrentTime) MOZ_OVERRIDE; > + // Override the GetBuffered() > + nsresult GetBuffered(dom::TimeRanges* aBuffered, "Override seek() ..." Add a newline after these functions. Add periods at the end of comments. Explain why GetBuffered does not return the buffered time ranges in the comment.

Thanks for the responses Benjamin - sorry for the delay in replying. (In reply to Benjamin Chen [:bechen] from comment #100) > (In reply to Steve Workman [:sworkman] from comment #90) > > Comment on attachment 746291 [details] [diff] [review] > > > > @@ +1327,5 @@ > > > + // If we call reset() first, the queue may still has some "garbage" frame > > > + // from another thread's |OnDataAvailable| before |SetFrameType|. > > > + void ResetWithFrameType(uint32_t aFrameType) { > > > + SetFrameType(aFrameType); > > > + Reset(); > > > > Should these be locked together? In this code, the lock is released between > > ... > > > The code flow is ok if the lock is released between these function calls. OK, I'll trust you on this one :) > > @@ +1402,5 @@ > > > + slots = (mSrcBufferDataLength[mConsumerIdx] / mSlotSize) + 1; > > > + // we have data, copy to aBuffer > > > + memcpy(aBuffer, > > > + (void *)(&mSrcBuffer[mSlotSize * mConsumerIdx]), > > > + mSrcBufferDataLength[mConsumerIdx]); > > > > If this is a ring buffer, then what if the data wraps around at the end of > > the array? > > The data would not wrap around the end of array, I add protection > |isReturnToHead| in WriteBuffer. > ex: if there is 1 slot left and the incoming data needs 2 slots, the > WriteBuffer will put the data to the begging of array. > Maybe I should add some check before memcpy. I would consider changing it so that the data wraps. You know the data size from the first slot, so it shouldn't be too complex to calculate how much data is at the end of the buffer and how much at the start. Now, this will result in two memcpys, but it should also result in less data discarding. It may be worthwhile to evaluate and compare fewer memcpys versus fewer discards. > > @@ +1490,5 @@ > > > … > > > + // we overwrite a slot that the decode thread doesn't retrieve > > > + // So the mConsumerIdx returns to head too and move forward to the oldest slot > > > > ... > Currently we have only one discard policy. Discard the oldest one frame. > And I don't know what happened if the I-Frame is missed, it depends the > decoder. > I guess the decoder still work properly but some block of the video are > wrong with color. > > If we need more discard policy (drop p/b frame..), we will need some "frame > sniff" for every codec type to detect the frame type. It's better to > implement them at libstagefright/rtsp/XXXAssembler.cpp. > And also we need a policy to turn on/off the discard action. (in > RtspTrackBuffer, we can observe the "overwrite" count. or the omx decoder > can return some status such as FPS) It may be simpler to discard as you do now, but don't deliver any frames until you reach an I-Frame. This way, the effect should be that the video pauses for an instant if a frame is dropped. Someone in the media team (e.g. Chris Pearce) should be able to help you figure out a good strategy here. However, my suggestion is to look at this later. Open a bug to "investigate impact of rtsp data discarding on I-Frames and media decoding" and assign it to yourself for follow-up. If it turns out there's no problem, just close it. > > @@ +1659,5 @@ > > > + mDecoder->SetTransportSeekable(seekable); > > > + } else { > > > + // live stream > > > + // enable realtime mode and create a realtime MediaDecoderStateMachine > > > + // Preferences::SetBool("media.realtime_decoder.enabled", true); > > > > Why is this comment about Preferences here? Do you intend to set it true, or > > do you mean to say it must be true for real-time streaming to proceed? > > > The preference "media.realtime_decoder.enabled" must be true for the live > stream. Code in MediaDecoderStateMachine constructor will overwrite > |mRealTime| it if the preference is false. > Or the playback won't succeed, it will show "black screen" due to the > MediaDecoderStateMachine expect to get the frame with timestamp, but the > live stream content doesn't contain timestamp. > > Should I check the preference and return some error? Or just leave it if the > preference is false? I'm not sure I understand completely. But here are some thoughts: -- If MediaDecoderStateMachine is already reading the preference and setting mRealTime, then read that var. -- Don't set the preference here. If it has been disabled by the user, there is a reason for that, and RTSP live streaming will be disabled. If the pref is disabled by default, then you should look into getting it enabled by default. This is an easy code change, but someone in the media team will have to ok it. > > @@ +1707,5 @@ > > > + > > > + if (mChannel) { > > > + if (aCloseImmediately) { > > > + // TODO: workaround here, we dont close the channel and listener now. > > > + //Close(); > > > > Out of interest, why do you not close the channel here? > > > The channel is dummy now, I will consider to remove the code about it. Yes, please remove any code that it no longer used, or comments that no longer apply.

Comment on attachment 746291 [details] [diff] [review] 2. Patch v07 - RtspMediaResource and MediaElement Review of attachment 746291 [details] [diff] [review]: ----------------------------------------------------------------- drive-by comments--I don't know this code well enough to provide feedback on patch in general. ::: content/media/MediaResource.cpp @@ +1285,5 @@ > + mTotalBufferSize(0), > + mFrameType(0), > + mIsStarted(false) { > + mSrcBuffer = (uint8_t*)malloc(BUFFER_SLOT_NUM * mSlotSize); > + if(!mSrcBuffer) { space between "if" and "(". I agree with Steve that you ought to switch to new() here or some other infallible allocator, unless we're talking a really large allocation here (many MBs). ::: content/media/MediaResource.h @@ +392,5 @@ > + return NS_OK; > + } > + // Seek to the given time offset. > + virtual nsresult SeekTime(int64_t aOffset) { > + return NS_OK; SeekTime is a no-op that returns OK. Please explain in a comment why that's ok. (If no one should call SeekTime on this class then use a failure code and/or MOZ_ASSERT?)

Hi, Jason, when are you available to review those patches what Vincent had provided before? One month has passed....:(

Comment on attachment 746291 [details] [diff] [review] 2. Patch v07 - RtspMediaResource and MediaElement Review of attachment 746291 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaResource.h @@ +769,5 @@ > + virtual int64_t GetNextCachedData(int64_t aOffset){return 0;} > + virtual int64_t GetCachedDataEnd(int64_t aOffset){return 0;} > + virtual bool IsDataCachedToEndOfResource(int64_t aOffset){return false;} > + > + class Listener MOZ_FINAL : public nsIInterfaceRequestor, Give class a more specific name: "Listener" is likely to conflict with something else somewhere.

I'm very sorry for taking so long here :( I'm on PTO until July 11th. I've asked Steve to look at these while I'm gone.

Hi Steve, may I know when are you available to review the patches ?

Hi Vincent - most likely next week. I'll be off for a few days this week (4th July is US Independence Day).

The other approach in my mind about RTSP is that we may use WebRTC APIs to support RTSP streaming. WebRTC provides APIs to handle RTP/RTCP related stuff. The signaling channel isn't defined in WebRTC, and user can select the ways to negotiate SDP. So we could implement the RTSP protocol in Gaia application, and use WebRTC APIs to handle RTP/RTCP.... Here is an example code in Gaia. var rtspChannel = createRtspChannel(url); var pc; var desc = {sdp}; // run start() to initiate a peer connection function start() { pc = new RTCPeerConnection(); // once remote stream arrives, show it in the remote video element pc.onaddstream = function (evt) { remoteView.src = URL.createObjectURL(evt.stream); }; } rtspChannel.onmessage = function (evt) { if (!pc) start(); var signal = JSON.parse(evt.data); if (signal.msgType === DESCRIBE) { pc.setRemoteDescription(new RTCSessionDescription(signal.sdp)); pc.createAnswer(pc.remoteDescription, gotDescription); function gotDescription(desc) { pc.setLocalDescription(desc); } } };

We have an e10s udp support in Bug 869869. Currently, the implementation of Rtsp and RTP/RTCP are in chrome process. Once bug 869869 is landed, we can move Rtsp stagefright stuff to content process and ease the IPC of RtspController. The advantages of this approach are 1. Don't need to handle android metadata via IPC, we can use it directly from android::MediaSource. 2. Handle the audio/video data in the accessunit basis. One rtp packet may contain several accessunits. Currently, the onMediaDataAvailable send the media data in packet base unit. However, we found that OMX decoder can't handle packet base unit well when audio format is AAC(mpeg4-generic). 3. Rtsp Stagefright stack may not be able to support various RTSP server. We can prevent the chance to crash chrome process if we move it to content.

Comment on attachment 750209 [details] [diff] [review] 4.Part0 Patch v05 - hook RtspProtocol to Necko Review of attachment 750209 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. I assume all of this parts will be merged together into one patch before landing? It's great for review to have multiple parts here, but it's better to have one patch for revision history later.

Comment on attachment 750211 [details] [diff] [review] 5. Part1 Patch v05 - Implement RtspChannel and RtspProtocolHandler Review of attachment 750211 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Some notes on comments and re-arranging function definitions. Also, please look into using moz.build as much as possible, for this patch and the others. ::: netwerk/protocol/rtsp/Makefile.in @@ +15,5 @@ > +FORCE_STATIC_LIB = 1 > + > +EXPORTS_NAMESPACES = mozilla/net > + > +CPPSRCS = \ I think you should put some of these definitions into moz.build. See netwerk/protocols/http/moz.build for examples of what you can move over. ::: netwerk/protocol/rtsp/RtspChannel.cpp @@ +42,5 @@ > + mListener = aListener; > + mListenerContext = aContext; > + > + // Reply the OnStartRequest directly to load media stream controller. > + // Most of jobs are done by media stream controller. "Call OnStartRequest directly. This will load the RtspMediaResource and create an streaming media controller. This controller manages the control and data streams to and from the network." ::: netwerk/protocol/rtsp/RtspChannel.h @@ +13,5 @@ > +namespace net { > + > +//----------------------------------------------------------------------------- > +// RtspChannel is designed for abstract channel and MediaResource creation > +// in HTMLMediaElement, and for passing the uri to RtspController. "RtspChannel is a dummy channel used to aid MediaResource creation in HTMLMediaElement. Actual network control and data flows are managed by an RtspController object created and owned by RtspMediaResource. Therefore, when RtspChannel::AsyncOpen is called, mListener->OnStartRequest will be called immediately. It is expected that an RtspMediaResource object will be created in that calling context or after; the RtspController object will be created internally by RtspMediaResource." @@ +23,5 @@ > + NS_DECL_ISUPPORTS > + > + RtspChannel(); > + ~RtspChannel(); > + NS_IMETHOD AsyncOpen(nsIStreamListener *listener, nsISupports *aContext); AsyncOpen overrides nsBaseChannel::AsyncOpen. So, please declare this: NS_IMETHOD AsyncOpen(nsIStreamListener *listener, nsISupports *aContext) MOZ_OVERRIDE MOZ_FINAL; Please add a comment to say why you're overriding it. @@ +27,5 @@ > + NS_IMETHOD AsyncOpen(nsIStreamListener *listener, nsISupports *aContext); > + NS_IMETHOD Init(nsIURI* uri); > + NS_IMETHOD OpenContentStream(bool aAsync, > + nsIInputStream **aStream, > + nsIChannel **aChannel); Please add a comment to say what RtspChannel does during Init and OpenContentStream. @@ +30,5 @@ > + nsIInputStream **aStream, > + nsIChannel **aChannel); > + NS_IMETHOD OnStartRequest(nsIRequest *aRequest, > + nsISupports *aContext); > + NS_IMETHOD OnStopRequest(nsIRequest *aRequest, Since none of these are actually implemented, add the function body in the header, i.e. { return NS_ERROR_NOT_IMPLMENTED; } @@ +38,5 @@ > + nsISupports *aContext, > + nsIInputStream *aInputStream, > + uint64_t aOffset, > + uint32_t aCount); > + NS_IMETHOD GetContentType(nsACString & aContentType); GetContentType also overrives nsBaseChannel::GetContentType; please add "MOZ_OVERRIDE MOZ_FINAL" to the end of the declaration. Please add a comment to say why you're overriding it. ::: netwerk/protocol/rtsp/RtspHandler.cpp @@ +59,5 @@ > +NS_IMETHODIMP > +RtspHandler::GetProtocolFlags(uint32_t *aProtocolFlags) > +{ > + *aProtocolFlags = URI_NORELATIVE | URI_NOAUTH | URI_INHERITS_SECURITY_CONTEXT | > + URI_LOADABLE_BY_ANYONE | URI_NON_PERSISTABLE | URI_IS_LOCAL_RESOURCE | URI_IS_LOCAL_RESOURCE seems wrong here according to the definition in nsIProtocolHandler. Is there a reason you're using it? @@ +67,5 @@ > +} > + > +NS_IMETHODIMP > +RtspHandler::NewURI(const nsACString & aSpec, const char *aOriginCharset, > + nsIURI *aBaseURI, nsIURI **_retval) Indentation. @@ +109,5 @@ > +} > + > +NS_IMETHODIMP > +RtspHandler::AllowPort(int32_t port, const char *scheme, > + bool *_retval) Indentation. ::: netwerk/protocol/rtsp/RtspHandler.h @@ +13,5 @@ > + > +namespace mozilla { > +namespace net { > + > +const static int32_t kDefaultRtspPort = 554; It's probably better to define this inside the class. Also, please have one space only before the '='. @@ +25,5 @@ > + RtspHandler(); > + virtual ~RtspHandler(); > + // Define a Create method to be used with a factory: > + static nsresult > + Create(nsISupports* aOuter, const nsIID& aIID, void* *aResult); I'm not sure if you need this Create function - you may be able to use NS_GENERIC_FACTORY_CONSTRUCTOR. ::: netwerk/protocol/rtsp/ipdl.mk @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +IPDLSRCS = \ Please move this into the first patch that contains IPDL files, so that IPDLSRCS is not empty in this patch.

Comment on attachment 750212 [details] [diff] [review] 6. Part2-1 patch v03 - original Android source code for RTSP and RTP Review of attachment 750212 [details] [diff] [review]: ----------------------------------------------------------------- Simple copy, so this should be fine. One question though - what is the license for these files? Is it ok to put them in netwerk/protocols/rtsp or should they be in the third-party directory?

Comment on attachment 750213 [details] [diff] [review] 7. Part2-2 patch v05 - Porting Rtsp to gecko Review of attachment 750213 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Some comments about naming, use of buffers, comments, unused code etc. ::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp @@ +418,5 @@ > + // Not sure why this is set to 3600 seconds ? But we can use > + // it to indicate that this is a live stream or not by setting > + // its value to zero. > + //mFormat->setInt64(kKeyDuration, 60 * 60 * 1000000ll); > + mFormat->setInt64(kKeyDuration, 0ll); Please clean this up. Also, if 3600 is arbitrary, just say so - is there a range of values that would do the same thing? ::: netwerk/protocol/rtsp/rtsp/MyHandler.h @@ +22,1 @@ > #define LOG_TAG "MyHandler" If you rename this to RtspConnectionHandler, please rename the log tag too. @@ +95,4 @@ > } > } > > struct MyHandler : public AHandler { Similar to MyTransmitter, can we call this RtspConnectionHandler? "My-" is very generic and I'd rather we use a more descriptive name. Can you rename this file too, please? @@ +183,5 @@ > msg->post(); > } > > + void play(uint64_t timeUs) { > + char buf[1024]; Do you need 1024 characters for the Range statement? @@ +214,5 @@ > + request.append("\r\n"); > + > + request.append("\r\n"); > + // Disable the access unit timeout until we resumed > + // playback again. *resume @@ +416,5 @@ > { > int32_t result; > CHECK(msg->findInt32("result", &result)); > > + LOGE("connection request completed with result %d (%s)", It looks like this will log all 'conn' messages received. This might pollute the console if you want to log errors only. Can you check result to see if it's an error first, then decide what log macro to use? There are other places you convert LOGI to LOGE - please review them and make changes as necessary. Concise logging will be very useful later. @@ +528,5 @@ > > + int64_t duration = 0; > + if (mSessionDesc->getDurationUs(&duration) > 0) { > + // This is not a live stream and therefore seekable. > + LOGE("This is not a live stream"); Is this really an error? @@ +673,5 @@ > setupTrack(index); > } else if (mSetupTracksSuccessful) { > ++mKeepAliveGeneration; > postKeepAlive(); > + #if 1 Why do you need this? If it's always going to be called, remove the #if. @@ +681,5 @@ > + } else { > + msg->setInt32("isSeekable", 0); > + } > + msg->setInt32("what", kWhatConnected); > + msg->post(); Why do you post this message? @@ +695,5 @@ > request.append("\r\n"); > > sp<AMessage> reply = new AMessage('play', id()); > mConn->sendRequest(request.c_str(), reply); > + #endif This code is not executed. Remove it if you're not going to use it. @@ +705,5 @@ > } > + case 'pause': > + { > + mPausePending = true; > + LOGE("pause completed"); Error? @@ +1006,5 @@ > request.append("Session: "); > request.append(mSessionID); > request.append("\r\n"); > > request.append("\r\n"); Looks like you removed a line here, but I think it's ok to leave that space. @@ +1028,5 @@ > > mNTPAnchorUs = -1; > > int64_t timeUs; > + char buf[256]; Here you use 256, but previously you used 1024 for a similar string. Can you decide what value is best and define a constant for it, please? @@ +1486,4 @@ > sp<AMessage> msg = mNotify->dup(); > msg->setInt32("what", kWhatConnected); > msg->post(); > + #endif Please remove this code if you're not using it. ::: netwerk/protocol/rtsp/rtsp/MyTransmitter.h @@ +17,1 @@ > #ifndef MY_TRANSMITTER_H_ Since we're making changes to these, can we rename MyTransmitter to RtspTransmitter? "My-" is very generic and I think we have an opportunity to be more descriptive here. Please rename the file too. ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp @@ +232,5 @@ > +void RTSPSource::performResume() { > +} > + > +void RTSPSource::performSuspend() { > +} Are performResume and performSuspend going to have code to do something? @@ +320,5 @@ > + info->mLatestPausedUnit = info->mLatestReceivedUnit; > + } > + > + // The latest timestamp we have in the enqueue when pasue operation is success. > + // Find the smallest one in tracks. "The timestamp after a 'Pause' is done is the earliest timestamp among all of the latest received units." @@ +581,5 @@ > (new AMessage)->postReply(mDisconnectReplyID); > mDisconnectReplyID = 0; > } > > +void PrintHex(const nsCString &aData) What is this used for? @@ +594,5 @@ > + } > + printf_stderr("%s\n", str); > +} > + > +void RTSPSource::onDataAvailable(size_t trackIndex) Can you rename this to onTrackDataAvailable, just to differentiate clearly between nsIStreamListener::OnDataAvailable and nsIStreamingProtocolListener::OnMediaDataAvailable :) @@ +618,5 @@ > + data.AssignLiteral("DISCONTINUITY"); > + if (mListener) { > + mListener->OnMediaDataAvailable(trackIndex, data, data.Length(), 0, meta.get()); > + } > + LOG(("To handle err:INFO_DISCONTINUITY")); "err:INFO_DISCONTINUITY" on its own is fine, or add some vars if you think it would be useful later for debugging, e.g. the trackIndex.

Comment on attachment 750214 [details] [diff] [review] 8. Part3 patch v05 - Streaming protocol service and controller interface Review of attachment 750214 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Some cleanup of comments to be done. ::: layout/build/nsLayoutModule.cpp @@ +1127,5 @@ > { &kOSFILECONSTANTSSERVICE_CID, true, NULL, OSFileConstantsServiceConstructor }, > { &kNS_ALARMHALSERVICE_CID, false, NULL, nsIAlarmHalServiceConstructor }, > { &kTCPSOCKETCHILD_CID, false, NULL, TCPSocketChildConstructor }, > { &kNS_TIMESERVICE_CID, false, NULL, nsITimeServiceConstructor }, > + { &kNS_MEDIASTREAMCONTROLLERSERVICE_CID, false, NULL, nsIStreamingProtocolControllerServiceConstructor }, Indentation seems off here. Please match &kNS_MEDIA.... ::: netwerk/base/public/nsIStreamingProtocolController.idl @@ +11,5 @@ > +#include "nsISupports.idl" > + > +%{C++ > +#define MEDIASTREAM_FRAMETYPE_NORMAL 0x00000001 > +#define MEDIASTREAM_FRAMETYPE_DISCONTINUNITY 0x00000002 ..._DISCONTINUITY @@ +26,5 @@ > + */ > + attribute uint32_t frameType; > + > + /** > + * The totol tracks for the given media stream session. *total @@ +28,5 @@ > + > + /** > + * The totol tracks for the given media stream session. > + */ > + attribute uint32_t tracks; numTracks or totalTracks to be clearer please. @@ +58,5 @@ > + > + /** > + * The timeStamp of the media stream. > + */ > + attribute unsigned long long timeStamp; Is this the current timestamp, as in the current time of the data coming from the server? Timestamp of data played could be a little different, right? "The current timestamp ..." @@ +63,5 @@ > + > + /** > + * The audio channel numbers of the media stream. > + */ > + attribute unsigned long channelCount; "The total number of audio channels in the media stream." @@ +68,5 @@ > + > + /** > + * The config of the track. > + */ > + attribute ACString ESDS; What is ESDS? What kind of config? What does it configure? @@ +73,5 @@ > + > + /** > + * The avcc of the track. > + */ > + attribute ACString AVCC; What is AVCC? @@ +87,5 @@ > + * Called when the data may be read without blocking the calling thread. > + * @param index The track number of the media stream. > + * @param data Raw data of the media stream on given track number. > + * @param length The length of the raw data. > + * @param offset The received raw data length. Offset is not normally used as length. Is this the offset in the data stream? Offset from where - from the start of the media presentation? In bytes? @@ +97,5 @@ > + in uint32_t offset, > + in nsIStreamingProtocolMetaData meta); > + > + /** > + * Called when the meta data for given session is availabled. *a* given session is *available*. @@ +102,5 @@ > + * @param index The track number of the media stream. > + * @param fd The socket pair id for inter process communication. > + * @param meta The meta data of the media stream. > + */ > + void onConnected(in uint8_t index, in int32_t fd, in nsIStreamingProtocolMetaData meta); Please remove the extra space before fd. @@ +105,5 @@ > + */ > + void onConnected(in uint8_t index, in int32_t fd, in nsIStreamingProtocolMetaData meta); > + > + /** > + * Called when Rtsp session is closed. when *the* RTSP session @@ +128,5 @@ > + /** > + * Asynchronously open this controller. Data is fed to the specified media stream > + * listener as it becomes available. If asyncOpen returns successfully, > + * the controller is responsible for keeping itself alive until it has called > + * onStopRequest on aListener. Good comment. Can you review the length of each line please? As many words as possible up to 80 chars. @@ +142,5 @@ > + */ > + nsIStreamingProtocolMetaData getTrackMetaData(in octet index); > + > + /* > + * Start to play. What does play mean at this layer? I imagine it is closely related to HTMLMediaElement::Play, but here it means start streaming data for playing - no? Please add something to comment to explain this. Same with other method comments here. @@ +175,5 @@ > + > + /** > + * Total audio/video track numbers. > + */ > + readonly attribute octet tracks; "Total number of audio/video tracks." tracks --> numTracks. ::: netwerk/base/public/nsIStreamingProtocolService.idl @@ +20,5 @@ > +[uuid(94838530-8627-11e2-9e96-0800200c9a66)] > +interface nsIStreamingProtocolControllerService : nsISupports > +{ > + /* > + * Create a new media stream controller from the given content type. ...for the given channel. @@ +21,5 @@ > +interface nsIStreamingProtocolControllerService : nsISupports > +{ > + /* > + * Create a new media stream controller from the given content type. > + * @param content Content type of the media stream. This comment seems out of date. ::: netwerk/base/src/StreamingProtocolService.cpp @@ +17,5 @@ > +namespace net { > + > +NS_IMPL_ISUPPORTS1(StreamingProtocolControllerService, nsIStreamingProtocolControllerService) > + > +StreamingProtocolControllerService::StreamingProtocolControllerService() If this constuctor doesn't do anything, you can add the implementation inline in StreamingProtocolService.h.

Comment on attachment 750216 [details] [diff] [review] 9. Part4 patch v05 - Rtsp controller, metadata and IPC Review of attachment 750216 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, although Jason is the real IPDL expert here. So, we should get him to take a look once you've dealt with my comments. ::: netwerk/ipc/NeckoChild.cpp @@ +154,5 @@ > return true; > } > > +PRtspControllerChild* > +NeckoChild::AllocPRtspController() Could these functions be wrapped in #ifdef MOZ_RTSP rather than just part of them? Seems like you wouldn't want them to be compiled at all, right? Same for NeckoParent.h/cpp @@ +158,5 @@ > +NeckoChild::AllocPRtspController() > +{ > +#ifdef MOZ_RTSP > + NS_NOTREACHED("AllocPRtspController should not be called"); > + return nullptr; You can pull this return nullptr; out of the #ifdef since it's also called in #else. @@ +170,5 @@ > +{ > +#ifdef MOZ_RTSP > + RtspControllerChild* p = static_cast<RtspControllerChild*>(child); > + p->ReleaseIPDLReference(); > + return true; You can pull this return true; out of the #ifdef since it's also called in #else. ::: netwerk/protocol/moz.build @@ +20,5 @@ > > if CONFIG['MOZ_RTSP']: > PARALLEL_DIRS += [ > 'rtsp', > ] Looks like a line was removed here, but nothing else. Please remove this change. ::: netwerk/protocol/rtsp/controller/RtspController.cpp @@ +53,5 @@ > +NS_IMPL_THREADSAFE_ISUPPORTS1(RtspController, > + nsIStreamingProtocolController) > + > +RtspController::RtspController(nsIChannel *channel) > +: mChannel(channel) Indentation. @@ +78,5 @@ > +NS_IMETHODIMP > +RtspController::Play(void) > +{ > + LOG(("RtspController::Play()")); > + if (mRtspSource != NULL) { Should anyone be calling Play if mRtspSource is not yet created? If this is not allowed, please change this to an NS_ENSURE_TRUE statement and fail. Or if you really think it should never happen, MOZ_ASSERT. Should you also check mState in these functions? Can Play be called if mState is not CONNECTED? ::: netwerk/protocol/rtsp/controller/RtspController.h @@ +32,5 @@ > + CONNECTED, > + DISCONNECTED > + }; > + > + nsCOMPtr<nsIURI> mURI; Some descriptions for these variables please. @@ +33,5 @@ > + DISCONNECTED > + }; > + > + nsCOMPtr<nsIURI> mURI; > + nsCOMPtr<nsIChannel> mChannel; Doesn't look like mChannel is used in the cpp - remove it? ::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp @@ +55,5 @@ > + > +RtspControllerChild::RtspControllerChild(nsIChannel *channel) > +: mIPCOpen(false) > +, mChannel(channel) > +, mTracks(0) Indentation. One tab in before the : and , @@ +76,5 @@ > +{ > + const char *data = aData.get(); > + char tmp[10], str[1024]; > + > + LOG(("printHex() data size: %d", aData.Length())); Indentation. The body of this function seems to be stepped in by one; please step it back. @@ +155,5 @@ > + for (uint32_t i = 0; i < metaArray.Length(); i++) { > + const MetaValue& value = metaArray[i].value(); > + const nsCString& name = metaArray[i].name(); > + > + if (name.EqualsLiteral("TRACKS")) { Looks like there is some overlap here with RecvOnMediaDataAvailable. Maybe you can pull out a common function - ArrayToMetadata? @@ +310,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +RtspControllerChild::OnConnected(uint8_t index, Could someone actually call this function? If not, please change the return type to NS_ERROR_NOT_IMPLEMENTED. Same for OnMediaDataAvailable and OnDisconnected. @@ +350,5 @@ > + > + rv = aURI->GetPort(&port); > + if (NS_FAILED(rv)) return rv; > + > + rv = aURI->GetAsciiSpec(mSpec); What if the spec is not rtsp? Should the function return an error? ::: netwerk/protocol/rtsp/controller/RtspControllerChild.h @@ +51,5 @@ > + void PrintHex(const nsCString &aData); > + > + private: > + bool mIPCOpen; > + nsCOMPtr<nsIChannel> mChannel; This is the dummy RtspChannel, right? Can you add some comments to describe each of these vars, please? ::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp @@ +70,5 @@ > + mController = new RtspController(nullptr); > + mController->Init(mURI); > + > + nsresult rv; > + rv = mController->AsyncOpen(this); You should be able to put these two lines together. If there is a compilation error on non debug builds, then use DebugOnly<nsresult> rv = ... Same with a lot of functions in this file. Please check the other files. @@ +90,5 @@ > + nsresult rv; > + > + rv = mController->Play(); > + > + if (NS_FAILED(rv)) return false; NS_ENSURE_SUCCESS(rv, false) will handle the error case AND put output on the console for easier debugging. I suggest you use this instead. @@ +172,5 @@ > + uint32_t length, > + uint32_t offset, > + nsIStreamingProtocolMetaData *meta) > +{ > + bool ret = false; ret is not used until the end of the function. Please declare it there. @@ +181,5 @@ > + > + // Serialize meta data. > + InfallibleTArray<MetaNamedValue> metaData; > + name.AssignLiteral("TIMESTAMP"); > + meta->GetTimeStamp(&int64Value); What if GetTimeStamp fails? You may need to have error handling here. Same for other metadata fields in this function and in OnConnected. @@ +186,5 @@ > + metaData.AppendElement(MetaNamedValue(name, int64Value)); > + > + name.AssignLiteral("FRAMETYPE"); > + meta->GetFrameType(&int32Value); > + metaData.AppendElement(MetaNamedValue(name, int32Value)); Looks like you only use TIMESTAMP and FRAMETYPE here, but in the child, you collect a lot more metadata types. I suggest you remove the code in the child for unused entries. @@ +190,5 @@ > + metaData.AppendElement(MetaNamedValue(name, int32Value)); > + > + stream.Assign(data); > + ret = SendOnMediaDataAvailable(index, stream, length, offset, metaData); > + return (ret ? NS_OK:NS_ERROR_NOT_AVAILABLE); Spaces around the : please. Same for other such statements in this file. ::: netwerk/protocol/rtsp/controller/RtspMetaData.cpp @@ +14,5 @@ > + > +NS_IMPL_ISUPPORTS1(RtspMetaData, nsIStreamingProtocolMetaData) > + > +RtspMetaData::RtspMetaData() > + : mIndex(0) Two spaces in front of the : please. @@ +28,5 @@ > + > +RtspMetaData::~RtspMetaData() > +{ > + > +} Remove empty line here. @@ +122,5 @@ > + mDuration = aDuration; > + return NS_OK; > +} > + > +/* attribute unsigned long sampleRate; */ Do you need this comment? If so, please add similar comments for each attribute. ::: netwerk/protocol/rtsp/controller/RtspMetaData.h @@ +25,5 @@ > + > + private: > + uint32_t mFrameType; > + uint32_t mIndex; > + uint32_t mTracks; mNumTracks @@ +30,5 @@ > + uint32_t mWidth; > + uint32_t mHeight; > + uint64_t mDuration; > + uint32_t mSampleRate; > + uint64_t mTimeStamp; mCurrentTimeStamp

General note on reviews: Some r-s, but you're almost there. Mostly, it seems like cleanup work, nothing major. Good work. (And very sorry you had to wait for the reviews for such a long time).

(In reply to Steve Workman [:sworkman] from comment #119) > General note on reviews: > > Some r-s, but you're almost there. Mostly, it seems like cleanup work, > nothing major. Good work. > > (And very sorry you had to wait for the reviews for such a long time). Thanks for your help to review the patches, happy to see the progress is moving on. I will try to update the patches and address your review comments soon.

(In reply to Steve Workman [:sworkman] from comment #115) > Comment on attachment 750212 [details] [diff] [review] > 6. Part2-1 patch v03 - original Android source code for RTSP and RTP > > Review of attachment 750212 [details] [diff] [review]: > ----------------------------------------------------------------- > > Simple copy, so this should be fine. One question though - what is the > license for these files? Is it ok to put them in netwerk/protocols/rtsp or > should they be in the third-party directory? These files are Apache License, Version 2.0. So it should be ok to put them in netwerk/protocols/rtsp, right ?

> ::: netwerk/ipc/NeckoChild.cpp > > > > +PRtspControllerChild* > > +NeckoChild::AllocPRtspController() > > Could these functions be wrapped in #ifdef MOZ_RTSP rather than just part of > them? Seems like you wouldn't want them to be compiled at all, right? > > Same for NeckoParent.h/cpp If I wrapped whole function in #ifdef MOZ_RTSP, the try server may complain in some builds. That's why I just wrapped part of them. > > @@ +78,5 @@ > > +NS_IMETHODIMP > > +RtspController::Play(void) > > +{ > > + LOG(("RtspController::Play()")); > > + if (mRtspSource != NULL) { > > Should anyone be calling Play if mRtspSource is not yet created? If this is > not allowed, please change this to an NS_ENSURE_TRUE statement and fail. Or > if you really think it should never happen, MOZ_ASSERT. The mRtspSource is declared as Android's strong pointer, and it seems not easy for me to use Marcos such as NS_ENSURE_TRUE or MOZ_ASSERT.

(In reply to Vincent Chang[:vchang] from comment #130) > > ::: netwerk/ipc/NeckoChild.cpp > > > > > > +PRtspControllerChild* > > > +NeckoChild::AllocPRtspController() > > > > Could these functions be wrapped in #ifdef MOZ_RTSP rather than just part of > > them? Seems like you wouldn't want them to be compiled at all, right? > > > > Same for NeckoParent.h/cpp > If I wrapped whole function in #ifdef MOZ_RTSP, the try server may complain > in some builds. That's why I just wrapped part of them. OK, understood. > > > > @@ +78,5 @@ > > > +NS_IMETHODIMP > > > +RtspController::Play(void) > > > +{ > > > + LOG(("RtspController::Play()")); > > > + if (mRtspSource != NULL) { > > > > Should anyone be calling Play if mRtspSource is not yet created? If this is > > not allowed, please change this to an NS_ENSURE_TRUE statement and fail. Or > > if you really think it should never happen, MOZ_ASSERT. > The mRtspSource is declared as Android's strong pointer, and it seems not > easy for me to use Marcos such as NS_ENSURE_TRUE or MOZ_ASSERT. Can you explain this a bit more please? MOZ_ASSERT(mRtspSource, "mRtspSource should not be null!"); seems easy enough to me ;) Also, if you decide not to use it, change the null checks to: if (mRtspSource) or if (!mRtspSource) ... unless strong pointers don't like that.

(In reply to Vincent Chang[:vchang] from comment #111 and comment #112) Vincent: Jason and I chatted briefly with jesup re the use of WebRTC for RTSP streaming. Here are his thoughts: jduell: sworkman: did you have any thoughts about https://bugzilla.mozilla.org/show_bug.cgi?id=831645#c111 [3:58pm] jduell: ie how the rtsp stuff could be supported by webRTC? [3:58pm] jduell: Maybe I should just cc :ekr and/or rjessup [4:00pm] sworkman: yeah - I saw it, but I assumed he still wanted reviews of the C++ code [4:00pm] sworkman: prob a good idea to copy either/both of those two [4:01pm] jduell: sworkman: yeah I don't understand well enough to give any advice on it. [4:01pm] annevk left the chat room. (Input/output error) [4:01pm] jesup: rtsp has nothing to with webrtc... rtsp is a streaming protocol [4:03pm] jduell: jesup: right, the idea seems to be to implement RSTP itself in gaia, and use webRTC APIs for RTP/RTCP. Do you have any thoughts on that? [4:03pm] sworkman: jesup: right - I think vchang was suggesting here https://bugzilla.mozilla.org/show_bug.cgi?id=831645#c111 that maybe the rtp/rtcp apis could be reused for rtsp [4:04pm] jesup: and we only handle RTP/RTCP via SRTP-DTLS - an encrypted RTP stream keyed via DTLS [4:04pm] jesup: RTSP normally assumes unencrypted RTP (and downstream only) [4:05pm] sworkman: I c - so it wouldn't really work then [4:05pm] jesup: There's RTP *code* that (with considerable work) might be used outside of webrtc, for receiving RTSP. But we don't expose this to JS [4:07pm] sworkman: I'll copy this conversation into the bug for vincent to read [4:08pm] sworkman: thanks jesup Considering the work that has been done so far, and the difficulties associated with using WebRTC, I think it's best to proceed with the current approach.

> > > > +NS_IMETHODIMP > > > > +RtspController::Play(void) > > > > +{ > > > > + LOG(("RtspController::Play()")); > > > > + if (mRtspSource != NULL) { > > > > > > Should anyone be calling Play if mRtspSource is not yet created? If this is > > > not allowed, please change this to an NS_ENSURE_TRUE statement and fail. Or > > > if you really think it should never happen, MOZ_ASSERT. > > The mRtspSource is declared as Android's strong pointer, and it seems not > > easy for me to use Marcos such as NS_ENSURE_TRUE or MOZ_ASSERT. > > Can you explain this a bit more please? MOZ_ASSERT(mRtspSource, "mRtspSource > should not be null!"); seems easy enough to me ;) > > Also, if you decide not to use it, change the null checks to: > > if (mRtspSource) or if (!mRtspSource) > > ... unless strong pointers don't like that. Thanks for your comments. Finally figure out the problem. I should use strong pointer's getter function to get the raw pointer. Something like "MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");" We can also use "if (mRtspSource.get())" to check if the pointer is NULL or not. I found some threading problems in debug build. I'll post a new patch to address them, too.

> Vincent: Jason and I chatted briefly with jesup re the use of WebRTC for > RTSP streaming. Here are his thoughts: > > jduell: sworkman: did you have any thoughts about > https://bugzilla.mozilla.org/show_bug.cgi?id=831645#c111 > [3:58pm] jduell: ie how the rtsp stuff could be supported by webRTC? > [3:58pm] jduell: Maybe I should just cc :ekr and/or rjessup > [4:00pm] sworkman: yeah - I saw it, but I assumed he still wanted reviews of > the C++ code > [4:00pm] sworkman: prob a good idea to copy either/both of those two > [4:01pm] jduell: sworkman: yeah I don't understand well enough to give any > advice on it. > [4:01pm] annevk left the chat room. (Input/output error) > [4:01pm] jesup: rtsp has nothing to with webrtc... rtsp is a streaming > protocol > [4:03pm] jduell: jesup: right, the idea seems to be to implement RSTP itself > in gaia, and use webRTC APIs for RTP/RTCP. Do you have any thoughts on that? > [4:03pm] sworkman: jesup: right - I think vchang was suggesting here > https://bugzilla.mozilla.org/show_bug.cgi?id=831645#c111 that maybe the > rtp/rtcp apis could be reused for rtsp > [4:04pm] jesup: and we only handle RTP/RTCP via SRTP-DTLS - an encrypted RTP > stream keyed via DTLS > [4:04pm] jesup: RTSP normally assumes unencrypted RTP (and downstream only) > [4:05pm] sworkman: I c - so it wouldn't really work then > [4:05pm] jesup: There's RTP *code* that (with considerable work) might be > used outside of webrtc, for receiving RTSP. But we don't expose this to JS > [4:07pm] sworkman: I'll copy this conversation into the bug for vincent to > read > [4:08pm] sworkman: thanks jesup > > Considering the work that has been done so far, and the difficulties > associated with using WebRTC, I think it's best to proceed with the current > approach. Thanks for your help to clarify this. Understood.

1. Rebase the patch to address the modification of THREADSAFE MACROs. 2. remove FORCE_STATIC_LIB = 1 in Makefile.in

Comment on attachment 778312 [details] [diff] [review] 9. Part4 patch v06 - Rtsp controller, metadata and IPC Review of attachment 778312 [details] [diff] [review]: ----------------------------------------------------------------- (vchang: I just noticed you've put up a newer version of this patch. But it sounds like the changes are minimal, so my comments here should still apply). This is looking good. Just a few questions, and some minor fixes. ::: netwerk/ipc/NeckoChild.cpp @@ +161,5 @@ > +{ > +#ifdef MOZ_RTSP > + NS_NOTREACHED("AllocPRtspController should not be called"); > +#endif > + return nullptr; Steve asked in the last round of review if we could #ifdef the entire definition of the functions here. IIUC it's hard to make IDPL code conditional (not sure if IPDL supports preprocessor in .ipdl files--I suspect not). So I think it's easier to just #ifdef the body of the functions (i.e. what you're already doing). But here, where you're only doing NOTREACHED/return null, it's fine to leave out the #ifdef--that code is fine for both cases. ::: netwerk/ipc/PNecko.ipdl @@ +59,5 @@ > PRemoteOpenFile(URIParams fileuri, nullable PBrowser browser); > > HTMLDNSPrefetch(nsString hostname, uint16_t flags); > CancelHTMLDNSPrefetch(nsString hostname, uint16_t flags, nsresult reason); > + PRtspController(); Big question: Are we ok with not identifying the app context here? I.e. not sending PBRowser/SerializedLoadContext? For HTTP/FTP/Websocket IPDL constructors we pass along a SerializedLoadContext and a PBrowser (the PBrowser we'll be getting rid of in bug 890570) in order to know which app is creating the channel (and if it's a browser, whether it's the browser app or browser content). The SerializedLoadContext also indicates whether the channel is for private browsing or not. The reason for all of that is to make sure that one app can't read/modify another app's cookies/localstorage/http cache/appcache/etc. And that we delete any private browsing info when the PB session ends. My understanding from talking with sworkman is that RTSP is using UDP to load the actual media content here, not HTTP. And we shouldn't be using cookies or localstorage or any other HTML/HTTP-related storage. In that case we should be OK to not do the PBrowser/SerializedLoadContext stuff for RSTP. But Steve and I wanted to make sure we asked you if there's any sort of state that RTSP keeps which needs to be protected and hidden from other apps. (The basic issue of not letting another app issue suspend/stop/play/etc commands on another apps' RTSP controller is AFAICT taken care of by the fact that it can't access another apps' PRtspController IPDL channel. So I'm asking about any sort of database lookups, etc that are made by either the RtspController or the code that loads the media from the server). For instance, are we using the media cache for Rtsp-controlled media? If we are, could the same rtsp:// URI for different apps result in different media content, so sharing a cache entry could be bad? We need to make sure there's no such issues in the whole codepath. ::: netwerk/protocol/rtsp/controller/PRtspController.ipdl @@ +12,5 @@ > + > +include protocol PBlob; //FIXME: bug #792908 > + > +include "SerializedLoadContext.h"; > +using IPC::SerializedLoadContext; Not using SerializedLoadContext in any IPDL msgs in this file, so can probably get rid of the include/using statements. ::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp @@ +25,5 @@ > +namespace mozilla { > +namespace net { > + > +nsresult > +ArrayToMetaData(const InfallibleTArray<MetaNamedValue>& metaArray, nsIStreamingProtocolMetaData *meta) Following the example of our URIParam IPDL classes, let's call this "DeserializeRtspMetaData" and either put it in a "util.cpp" file of some sort, or (maybe better) make it a member function of your RtspMetaData class. (Or we could switch away from using the union-based metadata, as I suggest later). @@ +111,5 @@ > + if (!gRtspChildLog) > + gRtspChildLog = PR_NewLogModule("nsRtspChild"); > +#endif > + AddIPDLReference(); > + gNeckoChild->SendPRtspControllerConstructor(this); Normally when you want the IPDL channel to be immediately created you'd just replace whatever calls RtspControllerChild* foo = new RtspControllerChild(channel) with the version of the SendPFooConstructor that does not take 'this': i.e. RtspControllerChild* foo = new PRtspControlllerConstructor(channel) but you've got this unusual approach where you're passing in an arg (channel) that isn't actually known by the IPDL constructor, so I guess that's fine. @@ +120,5 @@ > + LOG(("RtspControllerChild::~RtspControllerChild()")); > +} > + > +bool > +RtspControllerChild::RecvOnMediaDataAvailable(const uint8_t& index, All the HTTP/FTP ::RecvFOO IPDL functions wound up needing to queue IPDL messages in order to avoid re-entrant listener calls (where the same listener could be in the middle of an OnStartRequest call, for instance, yet have it's OnDataAvailable function also called before OnStart was complete). This was due to synchronous XmlHttpRequests being possible from the listener code, and/or from some JS wrapper stuff (which may or may not exist any more, or work differently--it's been a few years). I assume you don't have sync XHR going on in rtsp so perhaps you don't need the queuing. If you start seeing weird, seemingly re-entrant bugs than remember this and try using ChannelEventQueue to queue IPDL messages on the child. @@ +154,5 @@ > + const FileDescriptor& fd, > + const InfallibleTArray<MetaNamedValue>& metaArray) > +{ > + int parentSocket; > + uint32_t tracks = 2; tracks is always 2? @@ +155,5 @@ > + const InfallibleTArray<MetaNamedValue>& metaArray) > +{ > + int parentSocket; > + uint32_t tracks = 2; > + parentSocket = fd.PlatformHandle(); I assume this is not going to compile on Win32, where PlatformHandle is not going to be an int. Is this file getting skipped on windows? @@ +189,5 @@ > +bool > +RtspControllerChild::RecvAsyncOpenFailed(const uint8_t& reason) > +{ > + LOG(("RtspControllerChild::RecvAsyncOpenFailed %d", reason)); > + return true; You need to add some kind of error handling here. Something should happen if AsyncOpen fails. @@ +254,5 @@ > +NS_IMETHODIMP > +RtspControllerChild::Suspend(void) > +{ > + LOG(("RtspControllerChild::Suspend()")); > + if (!mIPCOpen || !SendSuspend()) { Is suspend/resume a recursive operation for RtspController? If so you're better off keeping a suspend count in the child and only suspending when it first gets >0, and only resuming when it hits 0 again. See HttpChannelChild::Suspend/Resume. @@ +284,5 @@ > +NS_IMETHODIMP > +RtspControllerChild::GetTotalTracks(uint8_t *aTracks) > +{ > + NS_ENSURE_ARG_POINTER(aTracks); > + *aTracks = 2; Again the magical 2 constant. Better to have a macro or const int 'kRstpTrackCount' variable somewhere, so in case this ever changes we don't have to 'grep 2' on the code :) ::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp @@ +41,5 @@ > +{ > +} > + > +bool > +RtspControllerParent::RecvDeleteSelf() It's not clear to me that you need the 'DeleteSelf' message. If you just had the child call Send__delete(), your ActorDestroy method should do the same mController->Stop, etc. logic that you have here. I'm not clear on why this separate message is needed (and why it nulls out mAuthProvider, but ActorDestroy doesn't). @@ +58,5 @@ > +{ > + LOG(("RtspControllerParent::ActorDestroy()")); > + mIPCOpen = false; > + > + NS_ENSURE_TRUE_VOID(mController); Assuming for some reason you can't get rid of the RecvDeleteSelf method: IIRC ActorDestroy gets called whenever the IPDL channel is torn down. If that's true, every time you do normal destruction RecvDeleteSelf will already have been called and thus mController will be null, right? You should only use the ENSURE macros if you want a warning to be emitted if the condition is false, but it sounds like it will generally be false here even when things are going correctly. @@ +74,5 @@ > + mController = new RtspController(nullptr); > + mController->Init(mURI); > + > + nsresult rv = mController->AsyncOpen(this); > + if (NS_FAILED(rv)) goto fail; I'm ok with goto statements (some people hate them) but you don't need one here: just if (NS_SUCCESS(rv)) return true, and fall down to the fail code otherwise. @@ +195,5 @@ > + uint64_t int64Value; > + > + LOG(("RtspControllerParent:: OnConnected")); > + // Serialize meta data. > + InfallibleTArray<MetaNamedValue> metaData; So you're using the Array<MetaNamedValue> in two different ways in OnConnected and OnMediaDataAvailable. It seems like you might be better off defining two different data types (OnConnectedMetadata and OnMediaAvailableMetaData)? Why do you need the generic type? @@ +282,5 @@ > + if (mAuthProvider && iid.Equals(NS_GET_IID(nsIAuthPromptProvider))) > + return mAuthProvider->GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL, > + iid, result); > + > + // Only support nsILoadContext if child channel's callbacks did too So right now nothing is setting mLoadContext, so you don't need it. This goes back to my question about whether we need to pass a SerializedLoadContext in the IPDL constructor. If not, we can get rid of mLoadContext. If we wind up keeping mLoadContext: IIRC the only reason we support some channels having a nsILoadContext and some not having it is for xpcshell testing (i.e. only xpcshell tests are allowed to not pass it). I'm guessing we'll always need a full browser to use Rtsp, so we should be able to omit 'mLoadContext' from the 'if' statement below. ::: netwerk/protocol/rtsp/controller/StreamingProtocolMetaData.ipdlh @@ +7,5 @@ > +namespace mozilla { > +namespace net { > + > +/** > + * Defined meta data format. As mentioned before, I'm not clear why you need the generic union type here instead of just having struct MediaDataAvailableInfo { uint64_t Timestamp; uint32_t FrameType; } (and another struct for OnConnected). I see your nsIStreamingProtocolMetaData class has a "addMetaData" function, so maybe there's something about rtsp that requires arbitrary dynamic field addition? But assuming you need the union, here are some comments. @@ +9,5 @@ > + > +/** > + * Defined meta data format. > + */ > +union MetaValue These are specific to Rtsp, right? (I.e. not used by anything but Rtsp code you're adding). If so let's call it "RtspMetaValue". Also, I believe you could put the definition into PRtspController.ipdl instead of a separate .ipdlh file. That's my understanding from reading https://developer.mozilla.org/en-US/docs/IPDL/Tutorial (ipdh files are for data types you need to use in more than file). @@ +21,5 @@ > + > +/** > + * Key-value pair. > + */ > +struct MetaNamedValue This could be called "RtspMetadataParam." I'm following the convention we use for our URI classes, where we have nsIURI for the XPCOM type and URIParam for the IPDL: http://mxr.mozilla.org/mozilla-central/source/ipc/glue/URIParams.ipdlh#63

Comment on attachment 780813 [details] [diff] [review] 9. Part4 patch v08 - Rtsp controller, metadata and IPC Review of attachment 780813 [details] [diff] [review]: ----------------------------------------------------------------- Looked over new version of the patch. Only calling IPDL on main thread is a very good idea :) Otherwise all my comments on v6 of the patch still apply. ::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp @@ +79,5 @@ > +NS_IMETHODIMP_(nsrefcnt) RtspControllerChild::Release() > +{ > + NS_PRECONDITION(0 != mRefCnt, "dup release"); > + // Enable this to find non-threadsafe destructors: > + // NS_ASSERT_OWNINGTHREAD(RtspControllerChild); Any reason not to just uncomment this?

Comment on attachment 778303 [details] [diff] [review] 2. Patch v08 - RtspMediaResource and MediaElement Review of attachment 778303 [details] [diff] [review]: ----------------------------------------------------------------- I haven't finished going through this yet, but since I'll be on vacation next week, I thought you should get these comments now. It seems like we're definitely getting to the clean up stage now. Keep up the good work. ::: content/media/MediaResource.cpp @@ +1290,5 @@ > mIgnoreResume = false; > } > } > > +/* class RtspTrackBuffer: a ring buffer implementation for audio/video track Please put all of the Rtsp code into its own file, RtspMediaResource.cpp. @@ +1316,5 @@ > + MOZ_COUNT_CTOR(RtspTrackBuffer); > +#ifdef PR_LOGGING > + mTrackIdx = aTrackIdx; > +#endif > + mSrcBuffer = new uint8_t[BUFFER_SLOT_NUM * mSlotSize]; Rename this to mRingBuffer. mSrcBuffer suggests that there is an mDestBuffer. @@ +1342,5 @@ > + void WriteBuffer(const char *aFromBuffer, uint32_t aWriteCount, > + uint64_t aFrameTime, uint32_t aFrameType); > + // Reset the mProducerIdx, mConsumerIdx, mBufferSlotDataLength[], > + // mBufferSlotDataTime[]. > + void Reset() { This looks big enough to be implemented outside the class. @@ +1382,5 @@ > + // should go forward. > + // 0(BUFFER_SLOT_EMPTY): The index slot is empty. mConsumerIdx should wait here. > + // positive value: The index slot contains valid data and the value is data size. > + int32_t mBufferSlotDataLength[BUFFER_SLOT_NUM]; > + uint64_t mBufferSlotDataTime[BUFFER_SLOT_NUM]; I think these two should be combined into a small struct. I kept reading mBufferSlotDataLength as a single int32_t, rather than an array. Call it BufferSlotData, and the variable mBufferSlotData. Then two fields, length and time. That should make it more clearly understood in the rest of the code. @@ +1410,5 @@ > + * early return and set the aFrameSize to notify the reader the aToBuffer > + * doesn't have enough space. The reader must realloc the aToBuffer if it > + * wishes to read the data. > + * */ > +nsresult RtspTrackBuffer::ReadBuffer(uint8_t* aToBuffer, uint32_t aToBufferSize, Function comment should go with the prototype in the class definition above. @@ +1415,5 @@ > + uint32_t* aReadCount, uint64_t *aFrameTime, > + uint32_t *aFrameSize) > +{ > + MonitorAutoLock monitor(mMonitor); > + if (aReadCount && aFrameTime && aFrameSize) { Please change this to NS_ENSURE_ARG error checks. Better to have one for each argument. Also, no need for RTSPMLOG here - NS_ENSURE_ARG will add error output to the console even if RTSPMLOG is disabled. NS_ENSURE_ARG(aReadCount); NS_ENSURE_ARG(aFrameTime); NS_ENSURE_ARG(aFrameSize); *aReadCount = 0; ... OR, if these parameters are required, why not pass by reference instead? uint32_t& aReadCount, uint64_t& aFrameTime... @@ +1425,5 @@ > + return NS_ERROR_NULL_POINTER; > + } > + RTSPMLOG("ReadBuffer mTrackIdx %d mProducerIdx %d mConsumerIdx %d " > + "mBufferSlotDataLength[mConsumerIdx] %d" > + ,mTrackIdx, mProducerIdx, mConsumerIdx These commas are fine at the end of the line. They're only useful at the start of the line in a constructor. @@ +1427,5 @@ > + RTSPMLOG("ReadBuffer mTrackIdx %d mProducerIdx %d mConsumerIdx %d " > + "mBufferSlotDataLength[mConsumerIdx] %d" > + ,mTrackIdx, mProducerIdx, mConsumerIdx > + ,mBufferSlotDataLength[mConsumerIdx]); > + while (1) { Please add a one line comment before while to explain what this loop does. @@ +1434,5 @@ > + if ((int32_t)aToBufferSize < mBufferSlotDataLength[mConsumerIdx]) { > + *aFrameSize = mBufferSlotDataLength[mConsumerIdx]; > + break; > + } > + uint32_t i; It should be ok to define 'i' in the for loop later. for (uint32_t i...) @@ +1436,5 @@ > + break; > + } > + uint32_t i; > + uint32_t slots = 1; > + slots = (mBufferSlotDataLength[mConsumerIdx] / mSlotSize) + 1; 'slots' is set after one line. Just set it once. @@ +1461,5 @@ > + RTSPMLOG("BUFFER_SLOT_INVALID move forward"); > + } else { > + // No data, and disconnected. > + if (!mIsStarted) { > + return NS_ERROR_FAILURE; Is this always an error? What if stop is pressed while the decoder is waiting to read, i.e. waiting at monitor.Wait()? When it wakes up, is it an error to return or is it expected behavior? @@ +1532,5 @@ > + slots = (aWriteCount / mSlotSize) + 1; > + } > + if (isMultipleSlots && > + (aWriteCount > (BUFFER_SLOT_NUM - mProducerIdx) * mSlotSize)) { > + isReturnToHead = true; This is ok to call returnToHead. @@ +1541,5 @@ > + // Clear the rest index of mBufferSlotDataLength > + for (i = mProducerIdx; i < BUFFER_SLOT_NUM; ++i) { > + mBufferSlotDataLength[i] = BUFFER_SLOT_INVALID; > + } > + // We overwrite a slot that the decode thread doesn't retrieve. We wrote over a slot that the decode thread has not yet read. @@ +1573,5 @@ > + } > + } > + mProducerIdx = (mProducerIdx + slots) % BUFFER_SLOT_NUM; > + // Move the mConsumerIdx forward to ensure that the decoder read the > + // "oldest" data. "...reads the oldest data available." ::: content/media/MediaResource.h @@ +387,5 @@ > + > + // Returns the nsIStreamingProtocolController in the MediaResource.(Rtsp) > + virtual nsIStreamingProtocolController* GetMediaStreamController() { > + return nullptr; > + } Instead of doing this, you could have RtspMediaResource implement nsIStreamingProtocolController, and use the NS_FORWARD_SAFE_NSISTREAMINGPROTOCOLCONTROLLER macro. It should be defined in <objdir>/dist/include/nsIStreamingProtocolController.h. That way you don't need to change MediaResource for these specific functions. See nsStreamListenerWrapper.h for an example. @@ +741,5 @@ > + * -> RtspMediaResource::SeekTime > + * */ > + > +class RtspTrackBuffer; > +class RtspMediaResource : public BaseMediaResource This is a big class definition and should be in its own file. Please create RtspMediaResource.h. @@ +751,5 @@ > + > + // assume that we only have one audio track > + // create them at |RtspMediaResource::OnConnected| > + nsCOMPtr<nsIStreamingProtocolController> mMediaStreamController; > + nsTArray<RtspTrackBuffer*> mTrackBuffer; Functions first, then variables in the class definition. @@ +753,5 @@ > + // create them at |RtspMediaResource::OnConnected| > + nsCOMPtr<nsIStreamingProtocolController> mMediaStreamController; > + nsTArray<RtspTrackBuffer*> mTrackBuffer; > + > + nsIStreamingProtocolController* GetMediaStreamController() MOZ_OVERRIDE { Please add comments to describe each of these new functions. @@ +772,5 @@ > + virtual void Suspend(bool aCloseImmediately); > + virtual void Resume(); > + virtual already_AddRefed<nsIPrincipal> GetCurrentPrincipal(); > + virtual bool CanClone() { > + // TODO: ?? Is this needed for the initial implementation? @@ +815,5 @@ > + virtual int64_t GetCachedDataEnd(int64_t aOffset) { return 0; } > + virtual bool IsDataCachedToEndOfResource(int64_t aOffset) { return false; } > + nsresult GetCachedRanges(nsTArray<MediaByteRange>& aRanges) { > + return NS_ERROR_FAILURE; > + } There are a lot of functions here which do nothing in this class, but serve a useful purpose in MediaResource. The class hierarchy should be changed so that these 'do nothing' implementations are not needed. @@ +841,5 @@ > + // These are called on the main thread by Listener. > + nsresult OnMediaDataAvailable(uint8_t aTrackIdx, const char *data, > + uint32_t length, > + nsIStreamingProtocolMetaData *meta); > + // The fd(ipc file descriptor) may be removed. Do you mean it can be omitted from a call to the function, or the parameter can be removed entirely from the prototype here? @@ +844,5 @@ > + nsIStreamingProtocolMetaData *meta); > + // The fd(ipc file descriptor) may be removed. > + nsresult OnConnected(uint8_t aTrackIdx, int32_t fd, > + nsIStreamingProtocolMetaData *meta = nullptr); > + nsresult OnDisconnected(uint8_t aTrackIdx, uint32_t aReason); These definitions look a lot like nsIStreamingProtocolListener - why don't you just have RtspMediaResource implement nsIStreamingProtocolListener and use the NS_DECL_NSISTREAMINGPROTOCOLLISTENER macro here?

Thanks for your elaborative comments, it's really helpful. Please see my reply inline below. > Big question: Are we ok with not identifying the app context here? I.e. not > sending PBRowser/SerializedLoadContext? > For HTTP/FTP/Websocket IPDL constructors we pass along a > SerializedLoadContext and a PBrowser (the PBrowser we'll be getting rid of > in bug 890570) in order to know which app is creating the channel (and if > it's a browser, whether it's the browser app or browser content). The > SerializedLoadContext also indicates whether the channel is for private > browsing or not. > > The reason for all of that is to make sure that one app can't read/modify > another app's cookies/localstorage/http cache/appcache/etc. And that we > delete any private browsing info when the PB session ends. > My understanding from talking with sworkman is that RTSP is using UDP to > load the actual media content here, not HTTP. And we shouldn't be using > cookies or localstorage or any other HTML/HTTP-related storage. In that RTSP uses TCP to send control message such as play, pause, teardown.... to the RTSP server. The RTSP server reply the media capability using the SDP format of the requested URI. The media capability includes the audio/video codec related information. The media content and network statistic for Qos are sent via RTP/RTCP protocol respectively using UDP. These information are not using cookies or local storage and any other HTML/HTTP related storage. > case we should be OK to not do the PBrowser/SerializedLoadContext stuff for > RSTP. But Steve and I wanted to make sure we asked you if there's any sort > of state that RTSP keeps which needs to be protected and hidden from other > apps. (The basic issue of not letting another app issue > suspend/stop/play/etc commands on another apps' RTSP controller is AFAICT > taken care of by the fact that it can't access another apps' PRtspController > IPDL channel. Is it possible that others app can access to another app's RTSP controller via IPC ? How ? AFAICT, RTSP server may response "401 Unauthorized" to ask the client to authenticate itself. The client may repeat the request with a suitable Authorization header field like HTTP. So we may need to pop-up a window to ask user enters username/password. Or the apps can have username/password fields and allow user to enter username and password. I'm not sure if it is must to do for RTSP. But Youtube seems not need this right now. May I do this in the follow-up bug. > So I'm asking about any sort of database lookups, etc that > are made by either the RtspController or the code that loads the media from > the server). For instance, are we using the media cache for Rtsp-controlled > media? If we are, could the same rtsp:// URI for different apps result in > different media content, so sharing a cache entry could be bad? We need to > make sure there's no such issues in the whole codepath. RTSP don't implement cache right now. > @@ +111,5 @@ > > + if (!gRtspChildLog) > > + gRtspChildLog = PR_NewLogModule("nsRtspChild"); > > +#endif > > + AddIPDLReference(); > > + gNeckoChild->SendPRtspControllerConstructor(this); > > Normally when you want the IPDL channel to be immediately created you'd just > replace whatever calls > > RtspControllerChild* foo = new RtspControllerChild(channel) > > with the version of the SendPFooConstructor that does not take 'this': i.e. > > RtspControllerChild* foo = new PRtspControlllerConstructor(channel) > > but you've got this unusual approach where you're passing in an arg > (channel) that isn't actually known by the IPDL constructor, so I guess > that's fine. So can I keep my current implementation or do I need to modify it ? > All the HTTP/FTP ::RecvFOO IPDL functions wound up needing to queue IPDL > messages in order to avoid re-entrant listener calls (where the same > listener could be in the middle of an OnStartRequest call, for instance, yet > have it's OnDataAvailable function also called before OnStart was complete). > This was due to synchronous XmlHttpRequests being possible from the listener > code, and/or from some JS wrapper stuff (which may or may not exist any > more, or work differently--it's been a few years). I assume you don't have > sync XHR going on in rtsp so perhaps you don't need the queuing. If you > start seeing weird, seemingly re-entrant bugs than remember this and try > using ChannelEventQueue to queue IPDL messages on the child. Thanks for sharing this information. The onMediaDataAvailable() should always be called after onConnected() function. The client gets the media information from server's SDP and generate the connected event. The connected event together with meta data are then send to content process. The decoder in content process gets the meta data and prepare the omx decoder . When decoder is ready, User is allowed to press the play button. The play event is passed to parent process and send to RTSP server. The RTSP server starts to send media data using RTP after receive play message. The RTP is received in parent process and passed to decoder via onMediaDataAvailable(). > > + const InfallibleTArray<MetaNamedValue>& metaArray) > > +{ > > + int parentSocket; > > + uint32_t tracks = 2; > > + parentSocket = fd.PlatformHandle(); > > I assume this is not going to compile on Win32, where PlatformHandle is not > going to be an int. Is this file getting skipped on windows? It is only compiled on b2g with flag MOZ_RTSP. > @@ +189,5 @@ > > +bool > > +RtspControllerChild::RecvAsyncOpenFailed(const uint8_t& reason) > > +{ > > + LOG(("RtspControllerChild::RecvAsyncOpenFailed %d", reason)); > > + return true; > > You need to add some kind of error handling here. Something should happen > if AsyncOpen fails. Can I use mListener->OnDisconnected() to notify the error ? > Is suspend/resume a recursive operation for RtspController? If so you're > better off keeping a suspend count in the child and only suspending when it > first gets >0, and only resuming when it hits 0 again. See > HttpChannelChild::Suspend/Resume. Yes, suspend/resume should be a recursive operation. I'll add the count to it. > > + > > + LOG(("RtspControllerParent:: OnConnected")); > > + // Serialize meta data. > > + InfallibleTArray<MetaNamedValue> metaData; > > So you're using the Array<MetaNamedValue> in two different ways in > OnConnected and OnMediaDataAvailable. It seems like you might be better > off defining two different data types (OnConnectedMetadata and > OnMediaAvailableMetaData)? Why do you need the generic type? The MetaData is the media related information for the media stream of the request uri. We would like to put the media related data except encoded frames to MetaData . So that the client(decoder) can use the unique interface to get them. I think generic type should be easier for the client. > > + */ > > +union MetaValue > > These are specific to Rtsp, right? (I.e. not used by anything but Rtsp code > you're adding). Is it possible that we may have different streaming protocol ?

> RTSP server may response "401 Unauthorized" to ask the client to authenticate > itself. The client may repeat the request with a suitable Authorization header > field like HTTP. So we may need to pop-up a window to ask user enters > username/password. So this is HTTP auth? (I'm not clear on if you're doing an actual HTTP request at some point). If so, then it would probably be the case that if app A sends username/password to site B, any other app could also use site B without providing a password. Probably something to fix eventually. But if we don't even have support for auth yet it doesn't need to block this code. Otherwise it sounds like you aren't using cookies/cache/etc, so for now I guess we can do without a SerializedLoadContext. > Is it possible that others app can access to another app's > RTSP controller via IPC? How? If there was some sort of token you pass along to perform an operation on a given controller ("tell controller Z to pause()") then that would be an issue. But it looks like you can only tell controller Z to pause if you hold the other end of the IPDL connection, so that should be ok. > So can I keep my current implementation or do I need to modify it ? Keep it. > Can I use mListener->OnDisconnected() to notify the error ? I don't know the RTSP client code well enough to say what the right code is if RecvAsyncOpenFailed() is hit. Something that lets the client code know that things failed, so perhaps OnDisconnected is good. > +union MetaValue For now give it a name that is RTSP-specific.

(In reply to Jason Duell (:jduell) from comment #143) > > RTSP server may response "401 Unauthorized" to ask the client to authenticate > > itself. The client may repeat the request with a suitable Authorization header > > field like HTTP. So we may need to pop-up a window to ask user enters > > username/password. > > So this is HTTP auth? (I'm not clear on if you're doing an actual HTTP > request at some point). If so, then it would probably be the case that if > app A sends username/password to site B, any other app could also use site B > without providing a password. Probably something to fix eventually. But if > we don't even have support for auth yet it doesn't need to block this code. > > Otherwise it sounds like you aren't using cookies/cache/etc, so for now I > guess we can do without a SerializedLoadContext. The authentication flow for RTSP is very similar to HTTP. But they are different. Here is a example. RTSP client sends DESCRIBE method to RTSP server for specific uri, RTSP server requests the client to authenticate itself using with status code 401 in reply. The client sends DESCRIBE to the server again with Authorization header. RTSP Client RTSP Server --> DESCRIBE rtsp://1.2.3.4/1.3gp RTSP/1.0 401 Unauthorized <-- --> DESCRIBE rtsp://1.2.3.4/1.3gp Authorization: Basic YWRtaW46YWRtaW4 > > +union MetaValue > > For now give it a name that is RTSP-specific. ok, got it. Can I use the generic type for metadata or I need to separate them to two structure in OnConnected and OnMediaDataAvailable ?

> Can I use the generic type for metadata or I need to separate them to two structure in OnConnected and OnMediaDataAvailable ? Either way is ok.

> The authentication flow for RTSP is very similar to HTTP. But they are different. OK, is there any shared database of auth info between apps? If not, I think we don't need SerializedLoadContext.