Closed Bug 831645 Opened 7 years ago Closed 6 years ago

Support rtsp streaming framework

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vchang, Assigned: vchang)

References

Details

(Keywords: feature, Whiteboard: RN5/29 [ucid:RTSP9, 1.3:p2, ft:ril])

Attachments

(16 files, 64 obsolete files)

42.79 KB, application/zip
Details
287.24 KB, image/jpeg
Details
30.47 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
17.04 KB, patch
Details | Diff | Splinter Review
303.31 KB, patch
Details | Diff | Splinter Review
49.85 KB, patch
Details | Diff | Splinter Review
17.77 KB, patch
Details | Diff | Splinter Review
52.94 KB, patch
Details | Diff | Splinter Review
36.42 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
875 bytes, patch
roc
: review+
Details | Diff | Splinter Review
1.85 KB, patch
gps
: review+
Details | Diff | Splinter Review
1.35 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.45 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
402.81 KB, patch
Details | Diff | Splinter Review
67.57 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → vchang
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.
Blocks: 844689
(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.
Blocks: 844712
(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 :-).
Attached patch wip, RtspMediaResource (obsolete) — Splinter Review
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.
Attached patch WIP, add rtsp protocol and ipc (obsolete) — Splinter Review
1. Create the rtsp protocol.
2. Hook the allocate/deallocate function to necko
Attachment #721137 - Flags: feedback?(sworkman)
1. create ipdl for ipc
2. implement nsIChannel and nsIStreamListener 
3. added nsRtspChannel/nsRtspChannelChild/nsRtspChannelParent
Attachment #721141 - Flags: feedback?(sworkman)
Attached patch WIP, rtsp in libstagefright (obsolete) — Splinter Review
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.
Attachment #721144 - Flags: feedback?(sworkman)
media stream controller interface which is used to control the media stream and retrieve meta data.
Attachment #721149 - Flags: feedback?(sworkman)
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.
Attachment #721137 - Flags: feedback?(sworkman) → review+
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.
Attachment #721141 - Flags: feedback?(sworkman) → feedback+
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.
Attached patch wip, RtspMediaResource (obsolete) — Splinter Review
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)
Attachment #718291 - Attachment is obsolete: true
Attached patch wip, RtspDecoder (obsolete) — Splinter Review
can't be compiled
- create RTSPDecoder, RTSPReader
- |class RTSPMediaSource : public MediaSource| in OmxDecoder.cpp
- |OmxDecoder::InitRTSP()|
Attachment #721616 - Flags: feedback?(tlee)
Attachment #721616 - Flags: feedback?(sworkman)
Attachment #721616 - Flags: feedback?(cku)
Attachment #721618 - Flags: feedback?(tlee)
Attachment #721618 - Flags: feedback?(sworkman)
Attachment #721618 - Flags: feedback?(cku)
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 :)
Attachment #721144 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #721149 - Flags: feedback?(sworkman)
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.
Attachment #721616 - Flags: feedback?(sworkman)
Attachment #721618 - Flags: feedback?(sworkman)
Attached patch WIP 2, Add RTSP protocol and IPC (obsolete) — Splinter Review
refactor the patch due to new file moz.build
Attachment #721137 - Attachment is obsolete: true
Attachment #727568 - Flags: feedback?(jduell.mcbugs)
1. The rtsp source will be created in mediastreamcontroller. 
2. Move the control functions to mediastreamcontroller. 
3. remove trail blank ...
Attachment #721141 - Attachment is obsolete: true
Attachment #727570 - Flags: feedback?(jduell.mcbugs)
The source comes from frameworks/base/media/libstagefright/rtsp in B2G tree.
Attachment #721144 - Attachment is obsolete: true
Attachment #727572 - Flags: feedback?(sworkman)
Attachment #727573 - Flags: feedback?(sworkman)
This patch includes the IPC code and new interfaces.
Attachment #721149 - Attachment is obsolete: true
Attachment #727575 - Flags: feedback?(sworkman)
Attachment #727575 - Flags: feedback?(jduell.mcbugs)
Attached patch Patch, compiler flag (obsolete) — Splinter Review
compiler flag for RTSP
Attachment #727580 - Flags: feedback?(sworkman)
Attached patch Patch 2, RTSP decoder (obsolete) — Splinter Review
Attachment #721618 - Attachment is obsolete: true
Attachment #721618 - Flags: feedback?(tlee)
Attachment #721618 - Flags: feedback?(cku)
Attachment #727581 - Flags: feedback?(sworkman)
Attachment #721616 - Attachment is obsolete: true
Attachment #721616 - Flags: feedback?(tlee)
Attachment #721616 - Flags: feedback?(cku)
Attachment #727582 - Flags: feedback?(sworkman)
Attached file RTSP test gaia app
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)
Attachment #727581 - Flags: feedback+
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.
Attachment #727580 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #727582 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #727581 - Flags: feedback?(sworkman) → feedback+
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;
Attachment #727573 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #727575 - Flags: feedback?(sworkman) → feedback+
(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.
Attachment #727575 - Attachment is obsolete: true
Attachment #727575 - Flags: feedback?(jduell.mcbugs)
Attachment #735121 - Flags: feedback?(sworkman)
Attachment #735121 - Flags: feedback?(jduell.mcbugs)
Attached patch Patch v04 - RTSP Decoder (obsolete) — Splinter Review
Attachment #727581 - Attachment is obsolete: true
Attachment #735123 - Flags: feedback?(sworkman)
Address the feedback comments and fix bugs.
Attachment #727582 - Attachment is obsolete: true
Attachment #735124 - Flags: feedback?(sworkman)
fix bug and add some somments
Attachment #735124 - Attachment is obsolete: true
Attachment #735124 - Flags: feedback?(sworkman)
Attachment #735614 - Flags: feedback?(sworkman)
bug fix
Attachment #735614 - Attachment is obsolete: true
Attachment #735614 - Flags: feedback?(sworkman)
Attachment #735644 - Flags: feedback?(sworkman)
Depends on: 856542
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
Attached image Draft architecture v0.1 (obsolete) —
(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.
Attachment #727572 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #727568 - Flags: feedback?(jduell.mcbugs) → feedback+
(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.
Attachment #735123 - Flags: feedback?(sworkman) → feedback+
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.
Attachment #735644 - Flags: feedback?(sworkman) → feedback+
(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.
Attachment #735121 - Flags: feedback?(sworkman)
Attachment #735121 - Flags: feedback?(jduell.mcbugs)
roc, does it seem likely to you that we'll ever want to control RTSP from worker JS?
Flags: needinfo?(roc)
AFAIK we wouldn't want to expose RTSP channels to JS at all.
Flags: needinfo?(roc)
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.
Attachment #727570 - Flags: feedback?(jduell.mcbugs)
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.
Attachment #735644 - Attachment is obsolete: true
Attached patch Patch v06 - RTSP Decoder (obsolete) — Splinter Review
Support live stream.
Separate the RTSPOmxDecoder and RTSPOmxReader into other files.
Attachment #735123 - Attachment is obsolete: true
Attachment #727580 - Attachment is obsolete: true
Attachment #746286 - Flags: review?(ted)
1. Switch the capitalization from RTSP to Rtsp
Attachment #745041 - Attachment is obsolete: true
Attachment #746291 - Flags: review?(sworkman)
Attached patch 3. Patch v07 - RtspDecoder (obsolete) — Splinter Review
1. Switch the capitalization from RTSP to Rtsp
2. Add MOZ_RTSP to Makefile.in and moz.build
Attachment #745042 - Attachment is obsolete: true
Attachment #746292 - Flags: review?(sworkman)
1. Add MOZ_RTSP to moz.build
2. Remove IPC for RtspChannel
Attachment #727568 - Attachment is obsolete: true
Attachment #746294 - Flags: review?(jduell.mcbugs)
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
Attachment #727570 - Attachment is obsolete: true
Attachment #746298 - Flags: review?(jduell.mcbugs)
This is copied from Android's libstagefright and should be the same with attach 727572(previous one). To be safe, attach it again.
Attachment #727572 - Attachment is obsolete: true
Attachment #746303 - Flags: review?(jduell.mcbugs)
1. Add MOZ_RTSP to toolkit/library/Makefile.in
Attachment #727573 - Attachment is obsolete: true
Attachment #746305 - Flags: review?(jduell.mcbugs)
Attachment #746305 - Attachment description: 6. Part2-2 patch v04 - Porting Rtsp to gecko → 7. Part2-2 patch v04 - Porting Rtsp to gecko
1. Switch the capitalization from RTSP to Rtsp
Attachment #735121 - Attachment is obsolete: true
Attachment #746309 - Flags: review?(jduell.mcbugs)
Attachment #746309 - Attachment description: 7. Part3 patch v04 - RtspController and IPC → 8. Part3 patch v04 - RtspController and IPC
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.
Attachment #746286 - Flags: review?(ted) → review-
Address the review comment.
Attachment #746286 - Attachment is obsolete: true
Attachment #747763 - Flags: review?(ted)
Comment on attachment 747763 [details] [diff] [review]
1. Patch v04 - Add MOZ_RTSP compiler flag

Review of attachment 747763 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #747763 - Flags: review?(ted) → review+
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.
Attachment #746291 - Flags: review?(sworkman) → review-
Attachment #747763 - Attachment is obsolete: true
rebase and address the try server errors
Attachment #750209 - Flags: review?(sworkman)
Attachment #750209 - Flags: review?(jduell.mcbugs)
Attachment #746294 - Attachment is obsolete: true
Attachment #746294 - Flags: review?(jduell.mcbugs)
rebase and address try server errors.
Attachment #746298 - Attachment is obsolete: true
Attachment #746298 - Flags: review?(jduell.mcbugs)
Attachment #750211 - Flags: review?(sworkman)
Attachment #750211 - Flags: review?(jduell.mcbugs)
Attachment #746303 - Attachment is obsolete: true
Attachment #746303 - Flags: review?(jduell.mcbugs)
Attachment #750212 - Flags: review?(sworkman)
Attachment #750212 - Flags: review?(jduell.mcbugs)
rebase and address try server errors
Attachment #746305 - Attachment is obsolete: true
Attachment #746305 - Flags: review?(jduell.mcbugs)
Attachment #750213 - Flags: review?(sworkman)
Attachment #750213 - Flags: review?(jduell.mcbugs)
1. streaming protocol service and controller interface.
2. address trying server errors.
Attachment #746309 - Attachment is obsolete: true
Attachment #746309 - Flags: review?(jduell.mcbugs)
Attachment #750214 - Flags: review?(sworkman)
Attachment #750214 - Flags: review?(jduell.mcbugs)
rebase and address trying server error.
Attachment #750216 - Flags: review?(sworkman)
Attachment #750216 - Flags: review?(jduell.mcbugs)
(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.
Attachment #746292 - Flags: review?(sworkman) → review-
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?)
Blocks: 877065
Whiteboard: RN5/29
Blocks: 877116
Blocks: 877193
Blocks: 878699
Blocks: 879670
Blocks: 884702
Hi, Jason, when are you available to review those patches what Vincent had provided before? One month has passed....:(
Flags: needinfo?(jduell.mcbugs)
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.
Flags: needinfo?(jduell.mcbugs)
Hi Steve, may I know when are you available to review the patches ?
Flags: needinfo?(sworkman)
Hi Vincent - most likely next week. I'll be off for a few days this week (4th July is US Independence Day).
Flags: needinfo?(sworkman)
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.
Attachment #750209 - Flags: review?(sworkman) → review+
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.
Attachment #750211 - Flags: review?(sworkman) → review-
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?
Attachment #750212 - Flags: review?(sworkman) → review+
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.
Attachment #750213 - Flags: review?(sworkman) → review-
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.
Attachment #750214 - Flags: review?(sworkman) → review-
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
Attachment #750216 - Flags: review?(sworkman) → review-
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).
Depends on: b2g-RTSP-1.3
Blocks: 892395
(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.
Summary: Support rtsp streaming → Support rtsp streaming framework
Blocks: b2g-RTSP-1.3
No longer depends on: b2g-RTSP-1.3
(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 ?
Attachment #750207 - Attachment is obsolete: true
Attachment #746291 - Attachment is obsolete: true
Attachment #778303 - Flags: review?(sworkman)
Attachment #778303 - Attachment description: 2. Patch v07 - RtspMediaResource and MediaElement → 2. Patch v08 - RtspMediaResource and MediaElement
Attached patch 3. Patch v08 - RtspDecoder (obsolete) — Splinter Review
Attachment #746292 - Attachment is obsolete: true
Attachment #778305 - Flags: review?(sworkman)
Attachment #750209 - Attachment is obsolete: true
Attachment #750209 - Flags: review?(jduell.mcbugs)
Attachment #750211 - Attachment is obsolete: true
Attachment #750211 - Flags: review?(jduell.mcbugs)
Attachment #778308 - Flags: review?(sworkman)
Attachment #750212 - Attachment is obsolete: true
Attachment #750212 - Flags: review?(jduell.mcbugs)
Attachment #750213 - Attachment is obsolete: true
Attachment #750213 - Flags: review?(jduell.mcbugs)
Attachment #778310 - Flags: review?(sworkman)
Attachment #750214 - Attachment is obsolete: true
Attachment #750214 - Flags: review?(jduell.mcbugs)
Attachment #778311 - Flags: review?(sworkman)
> ::: 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.
Attachment #750216 - Attachment is obsolete: true
Attachment #750216 - Flags: review?(jduell.mcbugs)
Attachment #778312 - Flags: review?(sworkman)
Attachment #778312 - Flags: review?(jduell.mcbugs)
Attachment #737361 - Attachment is obsolete: true
(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.
Attachment #778312 - Attachment is obsolete: true
Attachment #778312 - Flags: review?(sworkman)
Attachment #778312 - Flags: review?(jduell.mcbugs)
Attachment #779676 - Flags: review?(sworkman)
Attachment #779676 - Flags: review?(jduell.mcbugs)
Blocks: 897486
1. Rebase the patch to address the modification of THREADSAFE MACROs. 
2. remove FORCE_STATIC_LIB = 1 in Makefile.in
Attachment #779676 - Attachment is obsolete: true
Attachment #779676 - Flags: review?(sworkman)
Attachment #779676 - Flags: review?(jduell.mcbugs)
Attachment #780813 - Flags: review?(sworkman)
Attachment #780813 - Flags: review?(jduell.mcbugs)
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
Attachment #778312 - Attachment is obsolete: false
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?
Attachment #780813 - Flags: review?(sworkman)
Attachment #780813 - Flags: review?(jduell.mcbugs)
Attachment #780813 - Flags: feedback+
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?
Blocks: 898864
Blocks: 898866
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.
(In reply to Jason Duell (:jduell) from comment #146)
> > 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.

We don't shared database of authentication info between apps right now. So I'll skip SerializedLoadContext for now.
Update the patch to address the review comments.
Attachment #778312 - Attachment is obsolete: true
Attachment #780813 - Attachment is obsolete: true
Attachment #788087 - Flags: review?(jduell.mcbugs)
(In reply to Steve Workman [:sworkman] from comment #141)
> 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.
> 
> @@ +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?
> 
I think it should return error in this case. Since the stop is called that means the connection is closed or tear down. I'll try to test some cases such as "the end of stream" , "network connection abort".

> 
> ::: 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.
> 

Current the RtspController is implement at netwerk/protocol/rtsp/controller/RtspController.cpp. (Patch 9)
The RtspMediaResource send play/pause commands through it.
I'm not sure it's a good design to let RtspMediaResource implement nsIStreamingProtocolController?

Or I can make the function GetMediaStreamController to be a rtsp specific fuinction and  static_cast the mResource to RtspMediaResource in RtspOmxReader/RtspOmxecoder when using these functions?

> 
> @@ +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.
> 

May I do this on a follow-up bug? I think the change is big enough since the MediaResource is designed for byte-range stream and using MediaCache.

> @@ +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?
> 

We will remove the fd entirely later.

> @@ +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?

We had a internal class RtspMediaResource::Listener that implements nsIStreamingProtocolListener. (reference to ChannelMediaResource)
Is that good to let RtspMediaResource implement nsIStreamingProtocolListener?
(In reply to Benjamin Chen [:bechen] from comment #149)
> (In reply to Steve Workman [:sworkman] from comment #141)
> > ::: 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.
> > 
> 
> Current the RtspController is implement at
> netwerk/protocol/rtsp/controller/RtspController.cpp. (Patch 9)
> The RtspMediaResource send play/pause commands through it.
> I'm not sure it's a good design to let RtspMediaResource implement
> nsIStreamingProtocolController?
> 
> Or I can make the function GetMediaStreamController to be a rtsp specific
> fuinction and  static_cast the mResource to RtspMediaResource in
> RtspOmxReader/RtspOmxecoder when using these functions?

So, my desire here is to limit the number of changes to MediaResource. I know it is awkward because we're trying to do so many things with this base class - Channel, File and now Rtsp. I'd like to try to keep MediaResource to have common functions only, as much as possible.

However, I agree with you. Having RtspMediaResource implement nsIStreamingProtocolController would make that class huge too. So, I have a suggestion. Let's add one function, MediaResource::GetRtspPointer. In MediaResource, it should return a nullptr. In RtspMediaResource, it returns a pointer to itself in the form of an RtspMediaResource. This would be like having an nsCOMPtr and calling do_QueryInterface, except we're not using interfaces here.

This way, we get to pass around a generic MediaResource pointer, which has a small interface, and then access all of RtspMediaResource's functionality. AND, we can avoid using static_cast.

What do you think about this approach?

> > @@ +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.
> > 
> 
> May I do this on a follow-up bug? I think the change is big enough since the
> MediaResource is designed for byte-range stream and using MediaCache.

Sure, that sounds good. Please file a bug.

> > @@ +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?
> > 
> 
> We will remove the fd entirely later.

Can you add to the comment explaining when you will remove, under what conditions.

> > @@ +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?
> 
> We had a internal class RtspMediaResource::Listener that implements
> nsIStreamingProtocolListener. (reference to ChannelMediaResource)
> Is that good to let RtspMediaResource implement nsIStreamingProtocolListener?

That's fine to do that. RtspMediaResource is the real listener anyway - the Listener class is just a small proxy.
Comment on attachment 778303 [details] [diff] [review]
2. Patch v08 - RtspMediaResource and MediaElement

Review of attachment 778303 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. More cleanup comments and some error checking/assertions needed.

::: content/media/MediaResource.cpp
@@ +1607,5 @@
> +    mMediaStreamController->AsyncOpen(mListener);
> +  }
> +#ifdef PR_LOGGING
> +  if (!gMediaResourceLog) {
> +    gMediaResourceLog = PR_NewLogModule("MediaResource");

This log init should be done in the base class, right? So, I don't think you need it here.

@@ +1618,5 @@
> +  RTSPMLOG("~RtspMediaResource");
> +  if (mListener) {
> +      // Kill its reference to us since we're going away
> +      mListener->Revoke();
> +    }

Indentation.

@@ +1671,5 @@
> +{
> +  NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
> +  NS_ASSERTION(aTrackIdx < mTrackBuffer.Length(), "ReadTrack index > mTrackBuffer");
> +
> +  return mTrackBuffer[aTrackIdx]->ReadBuffer(aBuffer, aBufferSize, aBytes,

Some MOZ_ASSERTS for aBuffer and other params could be useful at the top of the function.

And a MOZ_ASSERT for mTrackBuffer[aTrackIdx]: check aTrackIdx < BUFFER_SLOT_MAX_SIZE and the array element is not nullptr.

(Please add MOZ_ASSERTS in the other functions for nullptr and out of bounds indexes).

@@ +1692,5 @@
> +nsresult
> +RtspMediaResource::OnConnected(uint8_t aTrackIdx, int32_t fd,
> +                               nsIStreamingProtocolMetaData *meta)
> +{
> +  if (!mIsConnected) {

Change this to:

if (mIsConnected) {
  return NS_OK;
}

It is clearer to see that there is no 'else' for this 'if'. And, you can remove one level of indentation for the rest of the function.

@@ +1701,5 @@
> +      nsCString rtspTrackId("RtspTrack");
> +      rtspTrackId.AppendInt(i);
> +      nsCOMPtr<nsIStreamingProtocolMetaData> trackMeta;
> +      mMediaStreamController->GetTrackMetaData(i, getter_AddRefs(trackMeta));
> +      trackMeta->GetDuration(&duration);

You should assert that trackMeta is not null before calling GetDuration.

@@ +1706,5 @@
> +
> +      // Here is a heuristic to estimate the slot size.
> +      // For video track, calculate the width*height.
> +      // For audio track, use the BUFFER_SLOT_DEFAULT_SIZE because the w*h is 0.
> +      // Finally clamp them into (BUFFER_SLOT_DEFAULT_SIZE,BUFFER_SLOT_MAX_SIZE)

Out of interest, is this heuristic working well?

@@ +1761,5 @@
> +
> +nsresult
> +RtspMediaResource::OnDisconnected(uint8_t aTrackIdx, uint32_t aReason)
> +{
> +  // TODO: error handling here for aReason.(mDecoder->NetworkError main-thread)

This error handling should be completed before the code lands.

@@ +1776,5 @@
> +
> +  MediaDecoderOwner* owner = mDecoder->GetMediaOwner();
> +  if (!owner) {
> +    // Shutting down; do nothing.
> +    return;

Looks like you don't use 'owner' - so, you can change this to:

if (!mDecoder->GetMediaOwner) { ...

Same for ::Resume.

@@ +1812,5 @@
> +
> +nsresult RtspMediaResource::Open(nsIStreamListener **aStreamListener)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +  // TODO:?

I don't think anything calls this function, right? For ChannelMediaResource the decoder calls it, but your decoder will do different things, correct? So, you can probably return NS_ERROR_NOT_IMPLEMENTED, or be even more strict and call MOZ_CRASH.

@@ +1851,5 @@
> +nsresult RtspMediaResource::SeekTime(int64_t aOffset)
> +{
> +  NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
> +
> +  LOG("Seek requested for aOffset [%lld] for decoder [%p]", aOffset, mDecoder);

RTSPLOG, right? Please check the other RtspMediaResource functions.

::: content/media/MediaResource.h
@@ +751,5 @@
> +
> +  // assume that we only have one audio track
> +  // create them at |RtspMediaResource::OnConnected|
> +  nsCOMPtr<nsIStreamingProtocolController> mMediaStreamController;
> +  nsTArray<RtspTrackBuffer*> mTrackBuffer;

Please use "nsTArray<nsAutoPtr<RtspTrackBuffer> > mTrackBuffer;" here. Then you won't need to call delete in the destructor.
Attachment #778303 - Flags: review?(sworkman) → review-
> And a MOZ_ASSERT for mTrackBuffer[aTrackIdx]: check aTrackIdx <
> BUFFER_SLOT_MAX_SIZE and the array element is not nullptr.

Oh - I got that wrong. I confused the trackIdx with the buffer slot index. Ignore that comment, sorry.
Comment on attachment 778305 [details] [diff] [review]
3. Patch v08 - RtspDecoder

Review of attachment 778305 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Cleanup, assertions etc.

::: content/media/MediaDecoder.cpp
@@ +1681,5 @@
> +bool
> +MediaDecoder::IsRtspEnabled()
> +{
> +  //Currently the Rtsp decoded by omx.
> +  return Preferences::GetBool("media.omx.enabled", false);

Will you add another pref for rtsp itself? This one is just for the decoder.

::: content/media/MediaDecoder.h
@@ +308,5 @@
>    }
>  
> +  void SetResource(MediaResource* resource)
> +  {
> +    mResource = resource;

Add a nullptr check here please.

::: content/media/omx/MediaOmxReader.h
@@ +70,5 @@
>  
>    virtual nsresult ReadMetadata(VideoInfo* aInfo,
>                                  MetadataTags** aTags);
>    virtual nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime);
> +  virtual nsresult GetBuffered(dom::TimeRanges* aBuffered, int64_t aStartTime);

Why do you need to make this change?

::: content/media/omx/RtspOmxDecoder.cpp
@@ +21,5 @@
> +  bool realTime = false;
> +  if (mResource) {
> +    realTime = mResource->IsRealTime();
> +  }
> +  return new MediaOmxStateMachine(this, new RtspOmxReader(this), realTime);

Could mResource be null here? If not, you should pass mResource->IsRealTime() as the last param for the state machine constructor.

@@ +24,5 @@
> +  }
> +  return new MediaOmxStateMachine(this, new RtspOmxReader(this), realTime);
> +}
> +
> +void RtspOmxDecoder::ApplyStateToStateMachine(PlayState aState)

Nice override :)

::: content/media/omx/RtspOmxReader.cpp
@@ +80,5 @@
> +  if (!mIsStarted) {
> +    // RtspMediaSource relinquish the ownership of MediaBuffer |buf| to mGroup.
> +    mGroup = new MediaBufferGroup();
> +    MediaBuffer *buf = new MediaBuffer(mFrameMaxSize);
> +    if (buf && mGroup) {

You shouldn't need this check: 'new' is infallible. If FF runs out of memory, it will crash.

@@ +112,5 @@
> +  if (out) {
> +    *out = nullptr;
> +  } else {
> +    return MEDIA_ERROR_BASE;
> +  }

Please change this to:
NS_ENSURE_TRUE(out, MEDIA_ERROR_BASE);
*out = nullptr;

This will put an error on the console when a nullptr is detected.

@@ +127,5 @@
> +  while (1) {
> +    err = mGroup->acquire_buffer(&mBuffer);
> +    if (err != OK) {
> +      return err;
> +    }

It might help with debugging later to change this to:

NS_ENSURE_TRUE(err == OK, err);

Since there are two places err is checked here, it will be useful to have console output to see which function call failed.

@@ +161,5 @@
> +}
> +
> +
> +// RtspExtractor is a custom extractor for Rtsp stream, unlike the other
> +// XXXExtractors are made for container media content.

s/unlike/whereas/

@@ +190,5 @@
> +RtspExtractor::RtspExtractor(MediaResource *resource)
> +  : mResource(resource)
> +{
> +  MOZ_COUNT_CTOR(RtspExtractor);
> +  mController = resource->GetMediaStreamController();

This is a small constructor. You can put it inline with the destructor.

Please add a null check/assert for resource.

@@ +216,5 @@
> +
> +sp<MetaData> RtspExtractor::getTrackMetaData(size_t index, uint32_t flag)
> +{
> +  if (index >= countTracks())
> +    return nullptr;

Looks like this is an error case. NS_ENSURE_TRUE(...) please.

@@ +225,5 @@
> +
> +  if (rtspMetadata) {
> +    // Convert msMeta into meta.
> +    // The getter function of nsIStreamingProtocolMetaData will set the
> +    // parameter to 0.

"... will initialize the metadata values to 0 before setting them."

@@ +243,5 @@
> +    rtspMetadata->GetDuration(&temp64);
> +    meta->setInt64(kKeyDuration, temp64);
> +
> +    nsCString tempCString;
> +    rtspMetadata->GetEsdsData( tempCString);

Whitespace before tempCString.

@@ +257,5 @@
> +}
> +
> +RtspOmxReader::RtspOmxReader(AbstractMediaDecoder* aDecoder)
> +  : MediaOmxReader(aDecoder)
> +{}

MOZ_CTOR please.

@@ +263,5 @@
> +nsresult RtspOmxReader::InitOmxDecoder()
> +{
> +  if (!mOmxDecoder.get()) {
> +    sp<MediaExtractor> extractor = new RtspExtractor(mDecoder->GetResource());
> +    mOmxDecoder = new OmxDecoder(mDecoder->GetResource(), mDecoder);

You should have assertion checks here for mDecoder and mDecoder->GetResource().

@@ +281,5 @@
> +  // RtspMediaResource.
> +  MediaResource *resource = mDecoder->GetResource();
> +  if (resource) {
> +    resource->SeekTime(aTime);
> +  }

Is it ok to continue if there is no resource here?

::: content/media/omx/RtspOmxReader.h
@@ +19,5 @@
> +class AbstractMediaDecoder;
> +
> +// RtspOmxReader is a subclass of MediaOmxReader but the RtspOmxDecoder is not a
> +// subclass of MediaOmxDecoder. RtspOmxDecoder inherit from MediaDecoder.
> +// The major reason that RtspOmxDecoder inherit from MediaOmxReader is the

Decoder inherits from Reader?? ;)

@@ +25,5 @@
> +// RtspOmxReader override these functions |InitOmxDecoder Seek GetBuffered| in
> +// Rtsp specific purpose.
> +// InitOmxDecoder : provide a Rtsp extractor.
> +// Seek : implement a time-based seek instead of byte-based.
> +// GetBuffered : empty now.

Good explanations, thank you. Can you add a bit to say why GetBuffered is empty now (even if it's just "... because data is not cached.").
Attachment #778305 - Flags: review?(sworkman) → review-
Comment on attachment 778308 [details] [diff] [review]
5. Part1 Patch v06 - Implement RtspChannel and RtspProtocolHandler

Review of attachment 778308 [details] [diff] [review]:
-----------------------------------------------------------------

Almost ready. Like the others, just some cleanup.

::: netwerk/build/nsNetModule.cpp
@@ -275,5 @@
>  
> -#ifdef MOZ_RTSP
> -#include "RtspHandler.h"
> -#endif
> -

Was there an RtspHandler.h before? I thought your file was new?

::: netwerk/protocol/rtsp/RtspChannel.cpp
@@ +21,5 @@
> +}
> +
> +RtspChannel::~RtspChannel()
> +{
> +}

Move the constructor and destructor into the header file please.

@@ +29,5 @@
> +//-----------------------------------------------------------------------------
> +
> +NS_IMETHODIMP
> +RtspChannel::AsyncOpen(nsIStreamListener *aListener, nsISupports *aContext)
> +{

MOZ_ASSERT(aListener);

I think aContext can be null, so you can ignore that one.

@@ +42,5 @@
> +  mListener = aListener;
> +  mListenerContext = aContext;
> +
> +  // Call OnStartRequest directly. This will load the RtspMediaResource and
> +  // create an streaming media controller. This controller manages the control

"Call OnStartRequest directly. mListener is expected to create/load an RtspMediaResource which will create an RtspMediaController. This controller ..."

@@ +57,5 @@
> +}
> +
> +NS_IMETHODIMP
> +RtspChannel::Init(nsIURI* aUri)
> +{

MOZ_ASSERT(aUri);

::: netwerk/protocol/rtsp/RtspChannel.h
@@ +36,5 @@
> +  NS_IMETHOD Init(nsIURI* uri);
> +  // Overrides nsBaseChannel::GetContentType, return streaming protocol type "RTSP".
> +  NS_IMETHOD GetContentType(nsACString & aContentType) MOZ_OVERRIDE MOZ_FINAL;
> +
> +  NS_IMETHOD OpenContentStream(bool aAsync,

This and the rest of the functions look like they should be 'MOZ_OVERRIDE MOZ_FINAL' as well.

::: netwerk/protocol/rtsp/RtspHandler.cpp
@@ +61,5 @@
> +  int32_t port;
> +
> +  nsresult rv = GetDefaultPort(&port);
> +  if (NS_FAILED(rv))
> +    return rv;

As with other error cases, I'd prefer you use NS_ENSURE_SUCCESS(rv, rv) here, because the consequent console output is very useful.

Please check other error cases in these patches.

@@ +65,5 @@
> +    return rv;
> +
> +  nsRefPtr<nsStandardURL> url = new nsStandardURL();
> +  rv = url->Init(nsIStandardURL::URLTYPE_AUTHORITY, port, aSpec,
> +                aOriginCharset, aBaseURI);

1 more space of indentation needed - very strict ;)

@@ +68,5 @@
> +  rv = url->Init(nsIStandardURL::URLTYPE_AUTHORITY, port, aSpec,
> +                aOriginCharset, aBaseURI);
> +  if (NS_FAILED(rv))
> +    return rv;
> +  NS_ADDREF(*_retval = url);

url.forget(_retval);

This avoids the extra AddRef/Release pair (caused by calling NS_ADDREF and url going out of scope). The refcount stays at 1 and the raw pointer is moved to the return value.

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#989

Can you rename _retval to aResult to be consistent with the other params in this function and the rest of the file?
Attachment #778308 - Flags: review?(sworkman) → review-
Comment on attachment 778310 [details] [diff] [review]
7. Part2-2 patch v06 - Porting Rtsp to gecko

Review of attachment 778310 [details] [diff] [review]:
-----------------------------------------------------------------

More cleanup comments. Almost there :)

::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
@@ +415,5 @@
>      if (sessionDesc->getDurationUs(&durationUs)) {
>          mFormat->setInt64(kKeyDuration, durationUs);
>      } else {
> +        // Set its value to zero to indicate that this is a live stream.
> +        mFormat->setInt64(kKeyDuration, 0ll);

This looks like you're setting kKeyDuration to '011' not zero. Is '011' a flag?

::: netwerk/protocol/rtsp/rtsp/ARTSPConnection.cpp
@@ +940,4 @@
>  
>      uint8_t key[16];
> +    CHECK_EQ(hashString.Length(), sizeof (key));
> +    memcpy(key, hashString.get(), hashString.Length());

You may be able to avoid this memcpy and access hashString's internal buffer:

-- Check that the string length is equal to 16; define a const for that.
-- Change 'key' from an array to a pointer.
-- Use GetData or GetMutableData (https://developer.mozilla.org/en-US/docs/nsCString#GetData) to set 'key' to point to hashString's internal buffer.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +20,3 @@
>  
> +#define LOG_NDEBUG 0
> +#define LOG_TAG "RtspConnectionHandler"

Please convert these logs to prlog format.

@@ +1027,5 @@
>                  request.append(mSessionID);
>                  request.append("\r\n");
> +                sprintf(buf, "Range: npt=%lld-\r\n", timeUs / 1000000ll);
> +                LOGI("Seek: Received PAUSE - Sending Play range %s\n", buf);
> +                request.append(buf);

You could also try using nsPrintfCString here. It looks like it is similar to StringPrintf.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +55,3 @@
>  
> +{
> +    mListener = aListener;

MOZ_ASSERT(aListener);

It shouldn't ever be null, right?

@@ +75,5 @@
>          mReflector = new AHandlerReflector<RTSPSource>(this);
>          mLooper->registerHandler(mReflector);
>      }
>  
>      CHECK(mHandler == NULL);

Consider converting CHECK to MOZ_ASSERT, NS_ENSURE_TRUE, or something similar.

@@ +450,5 @@
>  }
>  
> +void RTSPSource::onConnected(bool isSeekable)
> +{
> +    int sockets[2];

Please initialize the socket values.

@@ +455,4 @@
>      CHECK(mAudioTrack == NULL);
>      CHECK(mVideoTrack == NULL);
>  
>      size_t numTracks = mHandler->countTracks();

MOZ_ASSERT(mHandler); at the top of the function.

@@ +457,5 @@
>  
>      size_t numTracks = mHandler->countTracks();
>      for (size_t i = 0; i < numTracks; ++i) {
>          int32_t timeScale;
>          sp<MetaData> format = mHandler->getTrackFormat(i, &timeScale);

MOZ_ASSERT(format);

@@ +495,5 @@
>          }
>  
> +        if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
> +          LOG(("RTSPStreamingProtocolController:: opening stream socket pair"));
> +          continue;

This looks like a failure.

-- The log message should say "error opening ...".
-- Can you include more info in the log message? Try using explain_socketpair?
-- Do you really want to continue getting sockets for the next track? Should we not return with failure in this case?

@@ +526,5 @@
> +          meta->SetHeight(int32Value);
> +        }
> +
> +        // Optional meta data.
> +        const void *data;

Declare this as a 'const char*'. I don't think you need to cast it to 'void*' for the findData functions.

@@ +532,5 @@
> +        size_t length = 0;
> +
> +        if (format->findData(kKeyESDS, &type, &data, &length)) {
> +          nsCString esds;
> +          esds.Assign((const char *)data, length);

I don't know these functions, but I assume that 'type' tells you what kind of data type it is returning in the void** (&data). If that assumption is correct, please add an assertion for 'type'.

@@ +543,5 @@
> +          meta->SetAvccData(avcc);
> +        }
> +
> +        if (mListener) {
> +          mListener->OnConnected(i, (int32_t)sockets[1], meta.get());

Is there a valid case when mListener could be null?

If it's always non-null, please MOZ_ASSERT here.
If it CAN be null, e.g. app is closing and this object is cleaning up, consider checking that earlier in the function to avoid unnecessary processing.

Please check the other callback functions in this file too.

@@ +557,2 @@
>      status_t err;
>      CHECK(msg->findInt32("result", &err));

MOZ_ASSERT(msg);

@@ +557,5 @@
>      status_t err;
>      CHECK(msg->findInt32("result", &err));
>      CHECK_NE(err, (status_t)OK);
>  
>      mLooper->unregisterHandler(mHandler->id());

MOZ_ASSERT(mLooper);
MOZ_ASSERT(mHandler);

@@ +569,5 @@
>      }
> +    if (mListener) {
> +      mListener->OnDisconnected(0, err);
> +    }
> +    mAudioTrack = NULL;

nullptr, please.

@@ +607,5 @@
> +        data.AssignLiteral("DISCONTINUITY");
> +        if (mListener) {
> +            mListener->OnMediaDataAvailable(trackIndex, data, data.Length(), 0, meta.get());
> +        }
> +        LOG(("err %d:INFO_DISCONTINUITY", trackIndex));

"err trackIndex[%d]:INFO..."

Otherwise, it looks like an error number and not the track index.

@@ +616,5 @@
> +    int64_t int64Value;
> +
> +    meta = new mozilla::net::RtspMetaData();
> +
> +    CHECK(accessUnit->meta()->findInt64("timeUs", &int64Value));

MOZ_ASSERT(accessUnit);

::: netwerk/protocol/rtsp/rtsp/RTSPSource.h
@@ +33,3 @@
>  struct ALooper;
>  struct AnotherPacketSource;
> +struct RtspConnectionHandler;

Thank you for renaming this :)

::: netwerk/protocol/rtsp/rtsp/RTSPTransmitter.h
@@ +17,3 @@
>  #ifndef MY_TRANSMITTER_H_
>  
>  #define MY_TRANSMITTER_H_

Rename the header guards please.

And if this file contains something called MyTransmitter, rename that too, please.

@@ +26,2 @@
>  #include <openssl/md5.h>
> +#endif

Do you need this? Is this file included in non-FFOS builds?
Attachment #778310 - Flags: review?(sworkman) → review-
Comment on attachment 778311 [details] [diff] [review]
8. Part3 patch v06 - Streaming protocol service and controller interface

Review of attachment 778311 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with the changes requested.

::: netwerk/base/public/nsIStreamingProtocolController.idl
@@ +8,5 @@
> +#include "nsISupports.idl"
> +
> +%{C++
> +#define MEDIASTREAM_FRAMETYPE_NORMAL          0x00000001
> +#define MEDIASTREAM_FRAMETYPE_DISCONTINUITY  0x00000002

Spacing: values are not aligned.

::: netwerk/base/public/nsIStreamingProtocolService.idl
@@ +10,5 @@
> +#include "nsISupports.idl"
> +
> +%{C++
> +#define NS_MEDIASTREAMCONTROLLERSERVICE_CID     { 0x94838530, 0x8627, 0x11e2, { 0x9e, 0x96, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 } }
> +#define MEDIASTREAMCONTROLLERSERVICE_CONTRACTID "@mozilla.org/mediastream/mediastreamcontrollerservice;1"

80 chars max per line please.

::: netwerk/base/src/StreamingProtocolService.cpp
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +NS_IMPL_ISUPPORTS1(StreamingProtocolControllerService, nsIStreamingProtocolControllerService)

80 chars per line max, please. Check the rest of the file.

::: netwerk/base/src/StreamingProtocolService.h
@@ +27,5 @@
> +  virtual ~StreamingProtocolControllerService() {};
> +  static already_AddRefed<StreamingProtocolControllerService> GetInstance();
> +
> +private:
> +  static StaticRefPtr<StreamingProtocolControllerService> sSingleton;

The main comment in nsStaticPtr.h warns against using StaticRefPtrs as class variables. Maybe keep it outside the class, defined in the .cpp only?
Attachment #778311 - Flags: review?(sworkman) → review+
Thanks for your help to review these patches. 

> > -#ifdef MOZ_RTSP
> > -#include "RtspHandler.h"
> > -#endif
> > -
> 
> Was there an RtspHandler.h before? I thought your file was new?
It's moving down together with NS_GENERIC_FACTORY_CONSTRUCTOR(RtspHandler) below.
(In reply to Steve Workman [:sworkman] from comment #155)
> ::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
> @@ +415,5 @@
> >      if (sessionDesc->getDurationUs(&durationUs)) {
> >          mFormat->setInt64(kKeyDuration, durationUs);
> >      } else {
> > +        // Set its value to zero to indicate that this is a live stream.
> > +        mFormat->setInt64(kKeyDuration, 0ll);
> 
> This looks like you're setting kKeyDuration to '011' not zero. Is '011' a
> flag?

No, it's a zero long long. 

> ::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
> @@ +20,3 @@
> >  
> > +#define LOG_NDEBUG 0
> > +#define LOG_TAG "RtspConnectionHandler"
> 
> Please convert these logs to prlog format.
There are bunches of LOG(I,E,V,W) function in android's rtsp implementation. Do I need to modify them to prlog either ? My modification is something like 

#ifdef LOGI 
#undef LOGI
#define LOGI(msg, ...) prlog(.......)  


> >          mReflector = new AHandlerReflector<RTSPSource>(this);
> >          mLooper->registerHandler(mReflector);
> >      }
> >  
> >      CHECK(mHandler == NULL);
> 
> Consider converting CHECK to MOZ_ASSERT, NS_ENSURE_TRUE, or something
> similar.

The CHECK is very similar with MOZ_ASSERT but in android way. The CHECK fail lead to running process crash. How do you think about others CHECK ? Should they be modified to MOZ_ASSERT either ? It's may not a good idea thinking about merge effort with newer android version.  


> > +void RTSPSource::onConnected(bool isSeekable)
> > +{
> > +    int sockets[2];
> 
> Please initialize the socket values.

I am going to remove these socket related stuff. It's used to do IPC things because of performance consideration. But looks like rtsp function works well without the sockepair. 

> 
> Declare this as a 'const char*'. I don't think you need to cast it to
> 'void*' for the findData functions.
Can I keep this declaration to void ? Then, I don't need to cast data to void in findData().
Address the review comments.
Attachment #778303 - Attachment is obsolete: true
Attachment #796531 - Flags: review?(sworkman)
Attached patch 3. Patch v09 - RtspDecoder (obsolete) — Splinter Review
Address the review comments.
Attachment #778305 - Attachment is obsolete: true
Attachment #796533 - Flags: review?(sworkman)
1. Patch in "4.Part0 Patch v06 rebase - hook RtspProtocol to Necko" is given a r+. I merge it to this patch. 

2. address the review comments.
Attachment #778306 - Attachment is obsolete: true
Attachment #778308 - Attachment is obsolete: true
Attachment #796538 - Flags: review?
Attachment #796538 - Attachment description: part1.RtspProtocolAndChannel.patch → 4. Part1 add Rtsp protocol and channel
Attachment #796538 - Flags: review? → review?(sworkman)
Attachment #778309 - Attachment description: 6. Part2-1 patch v03 - original Android source code for RTSP and RTP → 5. Part2-1 patch v03 - original Android source code for RTSP and RTP
address the review comments.
Attachment #778310 - Attachment is obsolete: true
Attachment #796546 - Flags: review?(sworkman)
Address the review comments and remove socketpair.
Attachment #778311 - Attachment is obsolete: true
Remove socketpair and rebase.
Attachment #788087 - Attachment is obsolete: true
Attachment #788087 - Flags: review?(jduell.mcbugs)
Attachment #796554 - Flags: review?(jduell.mcbugs)
Attachment #796555 - Flags: review?(sworkman)
(In reply to Vincent Chang[:vchang] from comment #165)
> Created attachment 796555 [details] [diff] [review]
> 9. Resource Implement StreamingProtocolListener

> > > @@ +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?
> > 
> > We had a internal class RtspMediaResource::Listener that implements
> > nsIStreamingProtocolListener. (reference to ChannelMediaResource)
> > Is that good to let RtspMediaResource implement nsIStreamingProtocolListener?

> That's fine to do that. RtspMediaResource is the real listener anyway - the Listener class is just a small > > proxy.

Hi Workman:
This patch let the RtspMediaResource implement the nsIStreamingProtocolListener. But we think there may be a cyclic reference problem here. 

In previous  version, the RtspMediaResource destructor will call the |Revoke()| function in the Listener to prevent error when the RtspMediaResource is destroyed but the Listener isn't.
(Resource holds the controller and the listener, and the controller holds the listener)
After apply this patch, RtspMediaResource holds a controller, but the controller also holds the RtspMediaResource by the |AsyncOpen| .

Do you have any suggestion about this?
Comment on attachment 796531 [details] [diff] [review]
2. Patch v09 - RtspMediaResource and MediaElement

Review of attachment 796531 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me - please make the final changes I requested.

::: content/media/RtspMediaResource.cpp
@@ +113,5 @@
> +  int32_t mConsumerIdx;
> +  // Because each slot's size is fixed, we need a array to record the real
> +  // data length and data time stamp.
> +
> +  // The value in mBufferSlotData[index].mLength represents:

Add an extra line after mConsumerIdx, and remove the line after "data time stamp". This should make it clearer that the comment relates to mBufferSlotData and not mConsumerIdx.

@@ +135,5 @@
> +  // Set true/false when |Start()/Stop()| is called.
> +  bool mIsStarted;
> +
> +  // The FrameType is sync to nsIStreamingProtocolController.h
> +  void SetFrameType(uint32_t aFrameType) {

Functions at top, variables at bottom of class definition please.

@@ +150,5 @@
> +  RTSPMLOG("ReadBuffer mTrackIdx %d mProducerIdx %d mConsumerIdx %d "
> +           "mBufferSlotData[mConsumerIdx].mLength %d"
> +           ,mTrackIdx ,mProducerIdx ,mConsumerIdx
> +           ,mBufferSlotData[mConsumerIdx].mLength);
> +  // Reader should skip the BUFFER_SLOT_INVALID.

"...skip slots with mLength==BUFFER_SLOT_INVALID."

@@ +167,5 @@
> +      uint32_t slots = (mBufferSlotData[mConsumerIdx].mLength / mSlotSize) + 1;
> +      // we have data, copy to aToBuffer
> +      memcpy(aToBuffer,
> +             (void *)(&mRingBuffer[mSlotSize * mConsumerIdx]),
> +             mBufferSlotData[mConsumerIdx].mLength);

I'm concerned about reading beyond the end of the buffer here. I know that you move mProducerIdx to the start of the buffer if the length of data to be written runs over the end. However, I'd still like a MOZ_ASSERT in here to ensure that the code runs correctly in debug builds. Basically, mBufferSlotData[mConsumerIdx].mLength <= amount of data after mRingBuffer[mSlotSize * mConsumerIdx].

@@ +231,5 @@
> +    RTSPMLOG("mTotalBufferSize < aWriteCount, incoming data is too large");
> +    return;
> +  }
> +  // Checking the incoming data's frame type.
> +  // If we receive MEDIASTREAM_FRAMETYPE_DISCONTINUNITY, clear the mFrameType

...DISCONTINUITY :)

@@ +339,5 @@
> +  , mRealTime(false)
> +{
> +  nsCOMPtr<nsIStreamingProtocolControllerService> mediaControllerService =
> +    do_GetService(MEDIASTREAMCONTROLLERSERVICE_CONTRACTID);
> +  if (mediaControllerService) {

What happens if mediaControllerService is null here? Looks like this shouldn't happen - MOZ_ASSERT please.

::: content/media/RtspMediaResource.h
@@ +77,5 @@
> +  // Get the RtspMediaResource pointer if this MediaResource is a
> +  // RtspMediaResource. For calling Rtsp specific functions.
> +  virtual RtspMediaResource* GetRtspPointer() MOZ_OVERRIDE MOZ_FINAL {
> +    return this;
> +  }

Thank you for creating the new files for RtspMediaResource, AND for creating this GetRtspPointer function. Looks much cleaner :)

@@ +115,5 @@
> +  }
> +  virtual already_AddRefed<MediaResource> CloneData(MediaDecoder* aDecoder) {
> +    return nullptr;
> +  }
> +  // dummy

Please remember to file a bug to rework the class hierarchy here. These dummy functions are fine for now, but they should be removed to keep the interface clean for RtspMediaResource.

@@ +185,5 @@
> +  friend class Listener;
> +
> +protected:
> +  // Main thread access only
> +  nsRefPtr<Listener> mListener;

Try to keep functions first, then all variables at the end of the class.

@@ +190,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.

Already removed, right?

@@ +206,5 @@
> +  //called.
> +  bool mIsConnected;
> +  // live stream
> +  bool mRealTime;
> +

Extra line. Please delete.
Attachment #796531 - Flags: review?(sworkman) → review+
Comment on attachment 796533 [details] [diff] [review]
3. Patch v09 - RtspDecoder

Review of attachment 796533 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there: Add a pref for "media.rtsp.enabled" - using the OMX pref is not enough. Then, I'll r+ it.

::: content/media/MediaDecoder.cpp
@@ +1703,5 @@
> +bool
> +MediaDecoder::IsRtspEnabled()
> +{
> +  //Currently the Rtsp decoded by omx.
> +  return Preferences::GetBool("media.omx.enabled", false);

I want to see another pref created - "media.rtsp.enabled". Then return the value of both OMX and rtsp prefs here.

::: content/media/MediaDecoder.h
@@ +679,5 @@
>    // notifies any thread blocking on this object's monitor of the
>    // change. Call on the main thread only.
>    void ChangeState(PlayState aState);
>  
> +  // Called by |ChangeState|, to change the state of state machine.

"... to update the state machine."

::: content/media/omx/OmxDecoder.h
@@ +180,5 @@
> +  // When we are creating the extractor, it will sniff the data read from
> +  // MediaResource to create the corresponding extractor in order to extract
> +  // the data from a container.
> +  // For Rtsp case, it doesn't have container so that we need to provide a
> +  // custom MediaExtractor.

"Data is read from the MediaResource to create a suitable extractor which extracts data from a container. Note: RTSP requires a custom extractor because it does not have a container."

::: content/media/omx/RtspOmxReader.cpp
@@ +167,5 @@
> +  RtspExtractor(RtspMediaResource *aResource)
> +    : mRtspResource(aResource) {
> +    MOZ_COUNT_CTOR(RtspExtractor);
> +    if (mRtspResource) {
> +      mController = mRtspResource->GetMediaStreamController();

aResource and mController shouldn't be null right? MOZ_ASSERT, please.
Attachment #796533 - Flags: review?(sworkman) → review-
Comment on attachment 796538 [details] [diff] [review]
4. Part1 add Rtsp protocol and channel

Review of attachment 796538 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'd like to come back to RtspChannel in a separate bug though. I'd like to explore other possibilities that will keep HTMLMediaElement simple, clean up the inheritance from nsBaseChannel, and figure out the right relationship with RtspController. But for now, it's fine, so let's move forward. r=me.
Attachment #796538 - Flags: review?(sworkman) → review+
Comment on attachment 796546 [details] [diff] [review]
6. Part2-2 - Porting Rtsp to gecko

Review of attachment 796546 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies - after our discussion on IRC re merging updates later, I have changed my mind on CHECK vs MOZ_ASSERT, NULL vs nullptr etc. Please keep using NULL and CHECK, if they are already used in a file. If you use CHECK, don't use MOZ_ASSERT - be consistent.

I also commented on several cases where empty lines were removed or added. Looks like these are unnecessary (apologies if I told you to add/remove them earlier - my focus now is on keeping the diff as small as possible to aid merges later).

These issues seem easy to fix, so r=me - please make those changes.

::: netwerk/protocol/rtsp/moz.build
@@ +31,5 @@
>      'RtspChannel.cpp',
>      'RtspHandler.cpp',
>  ]
>  
> +

Extra line added. Please remove it.

::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
@@ +415,5 @@
>      if (sessionDesc->getDurationUs(&durationUs)) {
>          mFormat->setInt64(kKeyDuration, durationUs);
>      } else {
> +        // Set its value to zero to indicate that this is a live stream.
> +        mFormat->setInt64(kKeyDuration, 0ll);

Just to be extra clear here, please change the comment to "... zero (long long) ...". Then folks like me won't read it wrong :)

::: netwerk/protocol/rtsp/rtsp/ARTSPConnection.cpp
@@ +944,5 @@
> +      return;
> +    }
> +
> +    const char *key;
> +    hashString.GetData(&key);   

Trailing whitespace at the end of this line.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +43,5 @@
> +extern PRLogModuleInfo* gRtspLog;
> +#define LOGI(msg, ...) PR_LOG(gRtspLog, PR_LOG_ALWAYS, (msg, ##__VA_ARGS__))
> +#define LOGV(msg, ...) PR_LOG(gRtspLog, PR_LOG_DEBUG, (msg, ##__VA_ARGS__))
> +#define LOGE(msg, ...) PR_LOG(gRtspLog, PR_LOG_ERROR, (msg, ##__VA_ARGS__))
> +#define LOGW(msg, ...) PR_LOG(gRtspLog, PR_LOG_WARNING, (msg, ##__VA_ARGS__))

For all of the LOGx macros you define, see if you can do this:

#define LOGI(msg, ...) PR_LOG(gRtspLog, PR_LOG_ALWAYS, ("RTSP: " msg, ##__VA_ARGS__))

This will make log output easier to differentiate from other logs. If it doesn't compile, don't worry about it.

@@ +1012,5 @@
>  
>                  mNTPAnchorUs = -1;
>  
>                  int64_t timeUs;
> +

Extra line added here. Please remove.

@@ +1024,5 @@
>                  request.append(mSessionID);
>                  request.append("\r\n");
> +                nsAutoCString range(nsPrintfCString("Range: npt=%lld-\r\n", timeUs / 1000000ll));
> +                LOGI("Seek: Received PAUSE - Sending Play range %s\n", range.get());
> +                request.append(range.get());

Cool - I'm glad this worked :)

Do you need the log here? Looks like you could remove the nsAutoCString and do:

request.append(
        nsPrintfCString(
             "Range: ....").get());

@@ +1475,3 @@
>              if (mSeekable) {
>                  for (size_t i = 0; i < mTracks.size(); ++i) {
>                      TrackInfo *info = &mTracks.editItemAt(i);

Line removed. Please keep it.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +63,5 @@
>  }
>  
> +RTSPSource::~RTSPSource()
> +{
> +    if (mLooper != nullptr) {

So, now that I know that you want to merge updates later, you might want to keep the opening '{' on the same line as the function name. And you probably want to keep NULL. Sorry for the extra work here.

@@ +79,5 @@
>          mReflector = new AHandlerReflector<RTSPSource>(this);
>          mLooper->registerHandler(mReflector);
>      }
>  
> +    MOZ_ASSERT(!mHandler.get());

Re CHECK or MOZ_ASSERT - as long as CHECK crashes the app and we can get a useful stack trace back to analyze, you can keep CHECK. Whichever one you decide, just use one in this file, i.e. all MOZ_ASSERTs or all CHECKs.

@@ +260,5 @@
>      return true;
>  }
>  
> +void RTSPSource::onMessageReceived(const sp<AMessage> &msg) {
> +   if (msg->what() == kWhatDisconnect) {

Looks like you deleted one space in front of the if - no other change. Please add it back to keep the diff clean.

@@ +269,5 @@
>          finishDisconnectIfPossible();
>          return;
>      } else if (msg->what() == kWhatPerformSeek) {
>          int32_t generation;
>          CHECK(msg->findInt32("generation", &generation));

Looks like deleting this line isn't important. Please add it back to keep the diff small. (There's one more of these).

@@ +473,5 @@
>          bool isAudio = !strncasecmp(mime, "audio/", 6);
>          bool isVideo = !strncasecmp(mime, "video/", 6);
>  
>          TrackInfo info;
> +        // Initialize trackinfo.

You don't need this comment.

@@ +480,5 @@
> +        info.mLatestPausedUnit   = 0;
> +        info.mTimeScale          = timeScale;
> +        info.mRTPTime            = 0;
> +        info.mNormalPlaytimeUs   = 0ll;
> +        info.mNPTMappingValid    = false;

Looks like just 4 lines were added here, but the diff suggests 4 were changed as well. Try to clean this up for the revision history, i.e. only show the added lines.

::: netwerk/protocol/rtsp/rtsp/RTSPTransmitter.h
@@ +25,1 @@
>  #include <openssl/md5.h>

You can restore these removed lines - it should restrict the diff to code changes only.
Attachment #796546 - Flags: review?(sworkman) → review+
(In reply to Vincent Chang[:vchang] from comment #158)
> (In reply to Steve Workman [:sworkman] from comment #155)
> > ::: netwerk/protocol/rtsp/rtsp/APacketSource.cpp
> > @@ +415,5 @@
> > >      if (sessionDesc->getDurationUs(&durationUs)) {
> > >          mFormat->setInt64(kKeyDuration, durationUs);
> > >      } else {
> > > +        // Set its value to zero to indicate that this is a live stream.
> > > +        mFormat->setInt64(kKeyDuration, 0ll);
> > 
> > This looks like you're setting kKeyDuration to '011' not zero. Is '011' a
> > flag?
> 
> No, it's a zero long long. 

Ah, of course. Sorry about that. '1' and 'l' look very similar on my screen :)

> > ::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
> > @@ +20,3 @@
> > >  
> > > +#define LOG_NDEBUG 0
> > > +#define LOG_TAG "RtspConnectionHandler"
> > 
> > Please convert these logs to prlog format.
> There are bunches of LOG(I,E,V,W) function in android's rtsp implementation.
> Do I need to modify them to prlog either ? My modification is something like 
> 
> #ifdef LOGI 
> #undef LOGI
> #define LOGI(msg, ...) prlog(.......)  

The modifications you have in your recent patches look good to me.

> > >          mReflector = new AHandlerReflector<RTSPSource>(this);
> > >          mLooper->registerHandler(mReflector);
> > >      }
> > >  
> > >      CHECK(mHandler == NULL);
> > 
> > Consider converting CHECK to MOZ_ASSERT, NS_ENSURE_TRUE, or something
> > similar.
> 
> The CHECK is very similar with MOZ_ASSERT but in android way. The CHECK fail
> lead to running process crash. How do you think about others CHECK ? Should
> they be modified to MOZ_ASSERT either ? It's may not a good idea thinking
> about merge effort with newer android version.  

Yes, merging for later updates is an issue. I wrote in other comments that you should decide which one to use and stick with it. So, use all CHECKs in a file, or all MOZ_ASSERTs - don't mix them.

> > > +void RTSPSource::onConnected(bool isSeekable)
> > > +{
> > > +    int sockets[2];
> > 
> > Please initialize the socket values.
> 
> I am going to remove these socket related stuff. It's used to do IPC things
> because of performance consideration. But looks like rtsp function works
> well without the sockepair. 

OK.

> > Declare this as a 'const char*'. I don't think you need to cast it to
> > 'void*' for the findData functions.
> Can I keep this declaration to void ? Then, I don't need to cast data to
> void in findData().

If you have to cast to (const void*) in findData, then you decide what to do.
(In reply to Benjamin Chen [:bechen] from comment #166)
> (In reply to Vincent Chang[:vchang] from comment #165)
> > Created attachment 796555 [details] [diff] [review]
> > 9. Resource Implement StreamingProtocolListener
> 
> > > > @@ +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?
> > > 
> > > We had a internal class RtspMediaResource::Listener that implements
> > > nsIStreamingProtocolListener. (reference to ChannelMediaResource)
> > > Is that good to let RtspMediaResource implement nsIStreamingProtocolListener?
> 
> > That's fine to do that. RtspMediaResource is the real listener anyway - the Listener class is just a small > > proxy.
> 
> Hi Workman:
> This patch let the RtspMediaResource implement the
> nsIStreamingProtocolListener. But we think there may be a cyclic reference
> problem here. 
> 
> In previous  version, the RtspMediaResource destructor will call the
> |Revoke()| function in the Listener to prevent error when the
> RtspMediaResource is destroyed but the Listener isn't.
> (Resource holds the controller and the listener, and the controller holds
> the listener)
> After apply this patch, RtspMediaResource holds a controller, but the
> controller also holds the RtspMediaResource by the |AsyncOpen| .
> 
> Do you have any suggestion about this?

Hmm. I didn't mean to suggest you should change or add any references. What I meant was just to simplify the class definition: RtspMediaResource should implement nsIStreamingProtocolListener; RtspMediaResource::Listener should also implement nsIStreamingProtocolListener - both of them can use the NS_DECL_NSISTREAMINGPROTOCOLISTENER macro. The references should not have changed - it should be how you described for the "previous version".

Does that make sense or did I misunderstand something?
Comment on attachment 796555 [details] [diff] [review]
9. Resource Implement StreamingProtocolListener

Review of attachment 796555 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/RtspMediaResource.h
@@ -186,4 @@
>  
>  protected:
> -  // Main thread access only
> -  nsRefPtr<Listener> mListener;

Oh, here's the problem :) I didn't mean for you to remove Listener. I just meant that you should have the class definition be simple for RtspMediaResource.

So, keep Listener, and have both RtspMediaResource and RtspMediaResource::Listener implement nsIStreamingProtocolListener.

In Listener, you should be able to change mResource to an nsIStreamingProtocolListener, since none of the other functions in RtspMediaResource should be called.
Attachment #796555 - Flags: review?(sworkman) → review-
the issue has been reported again on FF0S1.1 during the Latam certification
(In reply to FangChen from comment #174)
> the issue has been reported again on FF0S1.1 during the Latam certification
This is out of scope of 1.1.
Comment on attachment 796554 [details] [diff] [review]
8. Part4 patch v09 - Rtsp controller, metadata and IPC

This looks just about ready AFAICT.  I only have minor changes.

>+NS_IMETHODIMP
>+RtspController::Play(void)
>+{
>+  LOG(("RtspController::Play()"));
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

It's good to add the MOZ_ASSERT in this function, but since it does nothing in
opt builds you also need the error return.  I.e change this to

  if (!mRtspSource.get()) {
    MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");
    return NS_ERROR_NOT_INITIALIZED;
  }

>+
>+  if (mState != CONNECTED) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRtspSource->play();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+RtspController::Pause(void)
>+{
>+  LOG(("RtspController::Pause()"));
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

ditto

>+
>+  if (mState != CONNECTED) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRtspSource->pause();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+RtspController::Resume(void)
>+{
>+  LOG(("RtspController::Resume()"));
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

Ditto

>+
>+  if (mState != CONNECTED) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRtspSource->play();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+RtspController::Suspend(void)
>+{
>+  LOG(("RtspController::Suspend()"));
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

Ditto

>+
>+  if (mState != CONNECTED) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRtspSource->pause();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+RtspController::Seek(uint64_t seekTimeUs)
>+{
>+  LOG(("RtspController::Seek() %llu", seekTimeUs));
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

Ditto

>+
>+  if (mState != CONNECTED) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  mRtspSource->seek(seekTimeUs);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+RtspController::Stop()
>+{
>+  LOG(("RtspController::Stop()"));
>+  mState = INIT;
>+  MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");

Ditto


>+bool
>+RtspControllerChild::RecvOnConnected(const uint8_t& index,
>+                                     const InfallibleTArray<RtspMetadataParam>& metaArray)

Make lines 80 chars max: I'd recommend something like

  RtspControllerChild::RecvOnConnected(
                        const uint8_t& index,
                        const InfallibleTArray<RtspMetadataParam>& metaArray)

I didn't mark all the places where you use >80 chars--fix the others too.

>+class RtspControllerParent : public PRtspControllerParent
>+                           , public nsIInterfaceRequestor
>+                           , nsIStreamingProtocolListener
>+{
>+ public:
>+  NS_DECL_THREADSAFE_ISUPPORTS

Are you sure you need THREADSAFE_ISUPPORTS for the parent class?  All the IPDL
methods (RecvPlay, etc) will only be called on the main thread.  I also assume
OnMediaDataAvailable, etc are also called on the main thread (otherwise they
would be erroneously calling IPDL SendFOO functions on a non-main thread).  If
there isn't any off-main thread use of the class, no need to make it THREADSAFE
AFAICT.


>+class RtspMetaData : public nsIStreamingProtocolMetaData
>+{
>+ public:
>+  NS_DECL_THREADSAFE_ISUPPORTS

I'm also not sure this needs to be THREADSAFE either.  AFAICT it's only used
during OnMediaDataAvailable and OnConnected, and I assume those are both
called only on the main-thread?  Is RtspMetaData used on any other thread?
Attachment #796554 - Flags: review?(jduell.mcbugs) → review-
> >+class RtspControllerParent : public PRtspControllerParent
> >+                           , public nsIInterfaceRequestor
> >+                           , nsIStreamingProtocolListener
> >+{
> >+ public:
> >+  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Are you sure you need THREADSAFE_ISUPPORTS for the parent class?  All the
> IPDL
> methods (RecvPlay, etc) will only be called on the main thread.  I also
> assume
> OnMediaDataAvailable, etc are also called on the main thread (otherwise they
> would be erroneously calling IPDL SendFOO functions on a non-main thread). 
> If
> there isn't any off-main thread use of the class, no need to make it
> THREADSAFE
> AFAICT.
> 
RtspControllerParent implements nsIStreamingProtocolListener interface, the interface will be used in Android thread to notify RtspControllerChild that the connection is established to Rtsp server. 

> >+class RtspMetaData : public nsIStreamingProtocolMetaData
> >+{
> >+ public:
> >+  NS_DECL_THREADSAFE_ISUPPORTS
> 
> I'm also not sure this needs to be THREADSAFE either.  AFAICT it's only used
> during OnMediaDataAvailable and OnConnected, and I assume those are both
> called only on the main-thread?  Is RtspMetaData used on any other thread?

Other than the main thread, the decoder thread needs to read the RtspMetaData to create omx decoder.
Update the patch to address review comments.
Attachment #796531 - Attachment is obsolete: true
Attached patch 3. Patch v10 - RtspDecoder (obsolete) — Splinter Review
Attachment #796533 - Attachment is obsolete: true
Attachment #797769 - Flags: review?(sworkman)
Comment on attachment 796555 [details] [diff] [review]
9. Resource Implement StreamingProtocolListener

Remove this patch per review comment.
Attachment #796555 - Attachment is obsolete: true
Address the review comments.
Attachment #796554 - Attachment is obsolete: true
Attachment #797774 - Flags: review?(jduell.mcbugs)
Comment on attachment 797769 [details] [diff] [review]
3. Patch v10 - RtspDecoder

Review of attachment 797769 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; r=me.
Attachment #797769 - Flags: review?(sworkman) → review+
Now that you have r+s from me, you need to get the code reviewed by Chris Pearce (or someone else that he suggests). He wanted to look over it before letting it land.
Blocks: 911627
Comment on attachment 797774 [details] [diff] [review]
8. Part4 - Rtsp controller, metadata and IPC

Review of attachment 797774 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #797774 - Flags: review?(jduell.mcbugs) → review+
Hi there, thanks all for review these patches, I'm going to rebase and push the patches to try server. Once I have fixed the errors reported by try server test, I'll post new patches and ask for core review by Chris Double. Hopefully, the try server errors could be fixed in one or two days.
Attached patch part0.RtspFlagMakefile.patch (obsolete) — Splinter Review
Attachment #778302 - Attachment is obsolete: true
Attachment #778309 - Attachment is obsolete: true
Attachment #796538 - Attachment is obsolete: true
Attachment #796546 - Attachment is obsolete: true
Attachment #796552 - Attachment is obsolete: true
Attachment #797767 - Attachment is obsolete: true
Attachment #797769 - Attachment is obsolete: true
Attachment #797774 - Attachment is obsolete: true
Attachment #798753 - Flags: review?(chris.double)
Attachment #798754 - Flags: review?(chris.double)
Attachment #798755 - Flags: review?(chris.double)
Attachment #798756 - Flags: review?(chris.double)
Attachment #798758 - Flags: review?(chris.double)
Attachment #798759 - Flags: review?(chris.double)
Comment on attachment 798754 [details] [diff] [review]
part1.RtspDecoer.patch

Review of attachment 798754 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with requested changes.

::: b2g/app/b2g.js
@@ +539,5 @@
>  // Enable system message
>  pref("dom.sysmsg.enabled", true);
>  pref("media.plugins.enabled", false);
>  pref("media.omx.enabled", true);
> +pref("media.rtsp.enabled", true);

You'll need to get a peer of b2g.js to review this portion. Put it in a separate patch for them to review.

::: content/media/DecoderTraits.cpp
@@ +255,5 @@
> +  if (!MediaDecoder::IsRtspEnabled()) {
> +    return false;
> +  }
> +
> +  return CodecListContains(gRtspTypes, aMimeType);

I think this is clearer as:

  return MediaDecoder::IsRtspEnabled() && CodecListContains(...);

enabling removal of the separate test.

@@ +263,5 @@
> +/* static */
> +bool DecoderTraits::DecoderWaitsForOnConnected(const nsACString& aMimeType) {
> +#ifdef MOZ_RTSP
> +  return CodecListContains(gRtspTypes, aMimeType);
> +#endif

#else

@@ +264,5 @@
> +bool DecoderTraits::DecoderWaitsForOnConnected(const nsACString& aMimeType) {
> +#ifdef MOZ_RTSP
> +  return CodecListContains(gRtspTypes, aMimeType);
> +#endif
> +  return false;

#endif

::: content/media/DecoderTraits.h
@@ +55,5 @@
>    // or false otherwise. Not all platforms support all MIME types, and
>    // vice versa.
>    static bool IsSupportedInVideoDocument(const nsACString& aType);
> +
> +  // Returns true if we should not start decoder immediately until we receive

Remove the word 'immediately'.

::: content/media/MediaDecoder.cpp
@@ +1186,5 @@
> +void MediaDecoder::ApplyStateToStateMachine(PlayState aState)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +

Instead of retaking the lock which the caller has already obtained, remove this lock and document in the comments for the function that it must be called with the lock obtained. Add an assertion here that the lock is held:

 GetReentrantMonitor().AssertCurrentThreadIn();

@@ +1700,5 @@
> +MediaDecoder::IsRtspEnabled()
> +{
> +  //Currently the Rtsp decoded by omx.
> +  return (Preferences::GetBool("media.rtsp.enabled", false) &&
> +          Preferences::GetBool("media.omx.enabled", false));

Replace the "media.omx.enabled" test with GetOmxEnabled() call.

::: content/media/MediaDecoder.h
@@ +681,5 @@
>    void ChangeState(PlayState aState);
>  
> +  // Called by |ChangeState|, to update the state machine.
> +  // Call on the main thread only.
> +  virtual void ApplyStateToStateMachine(PlayState aState);

MOZ_OVERRIDE

@@ +767,5 @@
>  
>  #ifdef MOZ_WEBM
>    static bool IsWebMEnabled();
>  #endif
> +#ifdef MOZ_RTSP

To be consistent with the other code, add a newline above this.

::: content/media/omx/MediaOmxReader.cpp
@@ +96,5 @@
>  
>    *aTags = nullptr;
>  
> +  // Initialize the internal OMX Decoder.
> +  NS_ENSURE_SUCCESS(InitOmxDecoder(), NS_ERROR_FAILURE);

If InitOmxDecoder() returns an error other than NS_ERROR_FAILURE then this information is lost by this statement. It doesn't return any error other than this at the moment but may in the future. Please restructure to something like:

  nsresult rv = InitOmxDecoder();
  if (NS_FAILED(rv)) return rv;

::: content/media/omx/OmxDecoder.h
@@ +171,5 @@
>  
>    // MediaResourceManagerClient::EventListener
>    virtual void statusChanged();
>  
> +  // The MediaExtractor provide essential information for creating OMXCodec

s/provide/provides/

::: content/media/omx/RtspOmxDecoder.cpp
@@ +17,5 @@
> +}
> +
> +MediaDecoderStateMachine* RtspOmxDecoder::CreateStateMachine()
> +{
> +  // Rtsp: support live stream

This comment doesn't really say much, it can be removed.

@@ +22,5 @@
> +  return new MediaOmxStateMachine(this, new RtspOmxReader(this),
> +                                  mResource->IsRealTime());
> +}
> +
> +void RtspOmxDecoder::ApplyStateToStateMachine(PlayState aState)

This method copies code from MediaDecoder::ApplyStateToStateMachine and risks falling out of date if changes are made to that method. I think it would be better to refactor it so that it calls the base class function then does the necessary additional actions.

@@ +25,5 @@
> +
> +void RtspOmxDecoder::ApplyStateToStateMachine(PlayState aState)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  ReentrantMonitorAutoEnter mon(GetReentrantMonitor());

The caller should have this lock already. Document in header that this is the case and add an assertion that lock is held:

  GetReentrantMonitor().AssertCurrentThreadIn();

@@ +33,5 @@
> +
> +  nsIStreamingProtocolController* controller =
> +    rtspResource->GetMediaStreamController();
> +  // If we have a nsIStreamingProtocolController, send play/pause commands here
> +  // except seek.

Please explain in comment why play/pause and not seek are sent and why this is done for an nsIStreamingProtocolController. In other words, explain why things are done the way they are done rather than explaining what the code does.

@@ +36,5 @@
> +  // If we have a nsIStreamingProtocolController, send play/pause commands here
> +  // except seek.
> +  // For PLAY_STATE_SEEKING state,
> +  // mDecoderStateMachine::Seek-> RtspOmxReader::Seek-> resource::SeekTime->
> +  // controller->Seek().

I don't understand this comment. Either expand or remove it.

::: content/media/omx/RtspOmxDecoder.h
@@ +10,5 @@
> +#include "MediaDecoder.h"
> +
> +namespace mozilla {
> +
> +class RtspOmxDecoder : public MediaDecoder

Comment explaining what this class does. Explain why it's not derived from MediaOmxDecoder.

@@ +22,5 @@
> +  ~RtspOmxDecoder() {
> +    MOZ_COUNT_DTOR(RtspOmxDecoder);
> +  }
> +
> +  virtual MediaDecoder* Clone();

Add MOZ_OVERRIDE to this and all other virtual functions in all classes in the patches where needed.

::: content/media/omx/RtspOmxReader.cpp
@@ +20,5 @@
> +#include <stagefright/MediaExtractor.h>
> +#include <stagefright/MediaBufferGroup.h>
> +#include <stagefright/MetaData.h>
> +
> +#define FRAME_DEFAULT_SIZE 1024

Explain what this is and where the number comes from.

@@ +66,5 @@
> +  uint32_t mTrackIdx;
> +  ReentrantMonitor mMonitor;
> +  bool mIsStarted;
> +
> +  // output for omx

What does this comment mean?

@@ +154,5 @@
> +// RtspExtractor is a custom extractor for Rtsp stream, whereas the other
> +// XXXExtractors are made for container media content.
> +// The extractor is used for |OmxDecoder::Init|, it provides the essential
> +// information for creating OMXCodec instance.
> +// For example, the |getTrackMetaData| returns a metadata that including the

s/a metadata/metadata/ and s/including/includes/

@@ +161,5 @@
> +{
> +public:
> +  size_t countTracks() MOZ_FINAL MOZ_OVERRIDE;
> +  sp<android::MediaSource> getTrack(size_t index) MOZ_FINAL MOZ_OVERRIDE;
> +  sp<MetaData> getTrackMetaData(size_t index, uint32_t flag = 0) MOZ_FINAL MOZ_OVERRIDE;

Add virtual to these.

@@ +170,5 @@
> +    MOZ_ASSERT(aResource);
> +    mController = mRtspResource->GetMediaStreamController();
> +    MOZ_ASSERT(mController);
> +  }
> +  ~RtspExtractor() {

Add virtual and MOZ_OVERRIDE.

::: content/media/omx/RtspOmxReader.h
@@ +19,5 @@
> +class AbstractMediaDecoder;
> +class RtspMediaResource;
> +
> +// RtspOmxReader is a subclass of MediaOmxReader but the RtspOmxDecoder is not a
> +// subclass of MediaOmxDecoder. RtspOmxDecoder inherit from MediaDecoder.

I don't think you should mention MediaOmxDecoder/RtspOmxDecoder here. Explain the reason in RtspOmxDecoder class file.

@@ +21,5 @@
> +
> +// RtspOmxReader is a subclass of MediaOmxReader but the RtspOmxDecoder is not a
> +// subclass of MediaOmxDecoder. RtspOmxDecoder inherit from MediaDecoder.
> +// The major reason that RtspOmxReader inherit from MediaOmxReader is the
> +// same video/audio decoding logic.

I don't understand this comment. If it's the same video/audio decoding logic you don't need to change it with a subclass.

@@ +23,5 @@
> +// subclass of MediaOmxDecoder. RtspOmxDecoder inherit from MediaDecoder.
> +// The major reason that RtspOmxReader inherit from MediaOmxReader is the
> +// same video/audio decoding logic.
> +// RtspOmxReader override these functions |InitOmxDecoder Seek GetBuffered| in
> +// Rtsp specific purpose.

Not need to list the functions - we can see it in the class definition and adds the burden of needing to update the comment when the code is updated. Just explain that it needs to add Rtsp specific functionality to OmxReader.

@@ +24,5 @@
> +// The major reason that RtspOmxReader inherit from MediaOmxReader is the
> +// same video/audio decoding logic.
> +// RtspOmxReader override these functions |InitOmxDecoder Seek GetBuffered| in
> +// Rtsp specific purpose.
> +// InitOmxDecoder : Provide a Rtsp extractor.

Add this comment to the actual function definition.

@@ +25,5 @@
> +// same video/audio decoding logic.
> +// RtspOmxReader override these functions |InitOmxDecoder Seek GetBuffered| in
> +// Rtsp specific purpose.
> +// InitOmxDecoder : Provide a Rtsp extractor.
> +// Seek : Implement a time-based seek instead of byte-based.

Add this comment to the actual function definition.

@@ +27,5 @@
> +// Rtsp specific purpose.
> +// InitOmxDecoder : Provide a Rtsp extractor.
> +// Seek : Implement a time-based seek instead of byte-based.
> +// GetBuffered : Empty now, because we don't cache it and the track buffer is
> +//               small. Also the data in track is not a/v sync.

Add this comment to the actual function definition.

@@ +45,5 @@
> +    mRtspResource = mDecoder->GetResource()->GetRtspPointer();
> +    MOZ_ASSERT(mRtspResource);
> +  }
> +
> +  ~RtspOmxReader() {

virtual and MOZ_OVERRIDE

@@ +50,5 @@
> +    MOZ_COUNT_DTOR(RtspOmxReader);
> +  }
> +
> +  // Override Seek() because Rtsp uses time-based seek function.
> +  nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime,

add 'virtual' keyword.

@@ +53,5 @@
> +  // Override Seek() because Rtsp uses time-based seek function.
> +  nsresult Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime,
> +                int64_t aCurrentTime) MOZ_FINAL MOZ_OVERRIDE;
> +
> +  // Override GetBuffered() to empty for below reasons:

The 'to empty' doesn't make sense here. Maybe you mean 'do nothing'.

@@ +61,5 @@
> +  // 2. Since the Rtsp is a realtime streaming, the buffer we made for
> +  // RtspMediaResource is quite small. The small buffer implies the time ranges
> +  // we returned are not useful for the MediaDecodeStateMachine. Unlike the
> +  // ChannelMediaResource, it has a "cache" that can store the whole streaming
> +  // data so the |GetBuffered| function can retrieve an useful time ranges.

s/an useful/useful/

@@ +62,5 @@
> +  // RtspMediaResource is quite small. The small buffer implies the time ranges
> +  // we returned are not useful for the MediaDecodeStateMachine. Unlike the
> +  // ChannelMediaResource, it has a "cache" that can store the whole streaming
> +  // data so the |GetBuffered| function can retrieve an useful time ranges.
> +  nsresult GetBuffered(mozilla::dom::TimeRanges* aBuffered,

add 'virtual' keyword.

@@ +68,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  RtspMediaResource* mRtspResource;

Document in a comment the lifetime and threading requirements of this.

::: content/media/omx/moz.build
@@ +27,5 @@
> +    CPP_SOURCES += [
> +        'RtspOmxDecoder.cpp',
> +        'RtspOmxReader.cpp',
> +    ]
> +

This needs to be reviewed a build peer. You should put it in a separate patch with all things build peers need to review.
Attachment #798754 - Flags: review?(chris.double) → review+
Comment on attachment 798753 [details] [diff] [review]
part0.RtspFlagMakefile.patch

Review of attachment 798753 [details] [diff] [review]:
-----------------------------------------------------------------

This needs to be reviewed by a build peer.
Attachment #798753 - Flags: review?(chris.double)
Comment on attachment 798755 [details] [diff] [review]
part2.RtspResourceAndMediaElement.patch

Review of attachment 798755 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for everything except for the stuff in RtspMediaResource.cpp which I'm struggling to understand. I'll review that separately (please put it in it's own patch and ask for review) so I don't hold up the review of the rest of this patch.

::: content/base/public/nsHostObjectProtocolHandler.h
@@ +13,5 @@
>  #define BLOBURI_SCHEME "blob"
>  #define MEDIASTREAMURI_SCHEME "mediastream"
>  #define MEDIASOURCEURI_SCHEME "mediasource"
>  #define FONTTABLEURI_SCHEME "moz-fonttable"
> +#define RTSPURI_SCHEME "rtsp"

You'll need to find a peer to review this file.

::: content/media/RtspMediaResource.cpp
@@ +49,5 @@
> +
> +class RtspTrackBuffer
> +{
> +public:
> +  RtspTrackBuffer(const char *monitor, int32_t aTrackIdx, uint32_t aSlotSize)

s/monitor/aMonitor/

@@ +59,5 @@
> +    MOZ_COUNT_CTOR(RtspTrackBuffer);
> +#ifdef PR_LOGGING
> +    mTrackIdx = aTrackIdx;
> +#endif
> +    mRingBuffer = new uint8_t[BUFFER_SLOT_NUM * mSlotSize];

Should this use mTotalBufferSize? The calculation can overflow if mSlotSize is large. Add an assert that ensures this won't happen. static_assert might be useful here.

@@ +117,5 @@
> +  // mConsumerIdx: A slot index that we read when decoder need(from OMX decoder).
> +  int32_t mProducerIdx;
> +  int32_t mConsumerIdx;
> +
> +  // Because each slot's size is fixed, we need a array to record the real

s/a array/an array/

::: content/media/RtspMediaResource.h
@@ +69,5 @@
> +{
> +public:
> +  RtspMediaResource(MediaDecoder* aDecoder, nsIChannel* aChannel, nsIURI* aURI,
> +                    const nsACString& aContentType);
> +  ~RtspMediaResource();

virtual/MOZ_OVERRIDE

@@ +71,5 @@
> +  RtspMediaResource(MediaDecoder* aDecoder, nsIChannel* aChannel, nsIURI* aURI,
> +                    const nsACString& aContentType);
> +  ~RtspMediaResource();
> +
> +  // Any thread

If this means "The following methods can be called on any thread" then say that. If it doesn't mean that, what does it mean?

@@ +79,5 @@
> +  virtual RtspMediaResource* GetRtspPointer() MOZ_OVERRIDE MOZ_FINAL {
> +    return this;
> +  }
> +
> +  // Returns the nsIStreamingProtocolController in the RtspMediaResource.

Explain in comment who owns it, lifetime and threading requirements.

@@ +84,5 @@
> +  nsIStreamingProtocolController* GetMediaStreamController() {
> +    return mMediaStreamController;
> +  }
> +
> +  bool IsRealTime() MOZ_OVERRIDE {

'virtual'

@@ +88,5 @@
> +  bool IsRealTime() MOZ_OVERRIDE {
> +    return mRealTime;
> +  }
> +
> +  // Other thread

What does this mean?

@@ +103,5 @@
> +
> +  // Seek to the given time offset
> +  nsresult SeekTime(int64_t aOffset);
> +
> +  // Main thread

MOZ_OVERRIDE where needed for all the methods below. Will any of these dummy implementations be called? If so, should they have ASSERTS? It seems strange to have a class that derives from a base but explicitly says it doesn't implement any of the required functions. Can you explain why you took this approach?

@@ +188,5 @@
> +
> +    void Revoke() { mResource = nullptr; }
> +
> +  private:
> +    RtspMediaResource* mResource;

Explain how ownership of this works.

@@ +197,5 @@
> +  // Main thread access only.
> +  // These are called on the main thread by Listener.
> +  NS_DECL_NSISTREAMINGPROTOCOLLISTENER
> +
> +  nsRefPtr<Listener> mListener;

Comment explaining what the listener is for.

@@ +200,5 @@
> +
> +  nsRefPtr<Listener> mListener;
> +
> +private:
> +  // These two members are created at |RtspMediaResource::OnConnected|.

Explain what are they for. Lifetime, threading, etc.

@@ +204,5 @@
> +  // These two members are created at |RtspMediaResource::OnConnected|.
> +  nsCOMPtr<nsIStreamingProtocolController> mMediaStreamController;
> +  nsTArray<nsAutoPtr<RtspTrackBuffer>> mTrackBuffer;
> +
> +  // A flag that indicates the |RtspMediaResource::OnConnected| had already been

s/had/has/

::: content/media/moz.build
@@ +74,5 @@
>      'MediaRecorder.h',
>      'MediaResource.h',
>      'MediaSegment.h',
>      'MediaStreamGraph.h',
> +    'RtspMediaResource.h',

This file needs to be reviewed by a build peer.
Attachment #798755 - Flags: review?(chris.double) → review+
Comment on attachment 798756 [details] [diff] [review]
part3.RtspProtocolAndChannel.patch

Review of attachment 798756 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs to be reviewed by a peer of the relevant modules.
Attachment #798756 - Flags: review?(chris.double)
Comment on attachment 798763 [details] [diff] [review]
part6.RtspControllerAndMetaData.patch

Review of attachment 798763 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs to be reviewed by a peer of the relevant modules.
Attachment #798763 - Flags: review?(chris.double)
Comment on attachment 798762 [details] [diff] [review]
part5.StreamingProtocolInterface.patch

Review of attachment 798762 [details] [diff] [review]:
-----------------------------------------------------------------

Needs netwerk peer review.
Attachment #798762 - Flags: review?(chris.double)
Comment on attachment 798759 [details] [diff] [review]
part4-2.RtspStagefright.patch

Review of attachment 798759 [details] [diff] [review]:
-----------------------------------------------------------------

Needs netwerk peer review.
Comment on attachment 798759 [details] [diff] [review]
part4-2.RtspStagefright.patch

Review of attachment 798759 [details] [diff] [review]:
-----------------------------------------------------------------

Needs netwerk peer review.
Attachment #798759 - Flags: review?(chris.double)
Comment on attachment 798758 [details] [diff] [review]
part4-1.RtspStagefright.patch

Review of attachment 798758 [details] [diff] [review]:
-----------------------------------------------------------------

This is all being added to netwerk so they'll need to review it if they haven't already.
Attachment #798758 - Flags: review?(chris.double)
Per Chris's comment on the patches. I listed the reviewers who given the patches r+ below. Not sure if we could commit these patches directly or we still need other peers to review the patches.  

part0.RtspFlagMakefile.patch            => r+ by ted.mielczarek
part3.RtspProtocolAndChannel.patch      => r+ by sworkman 
part4-1.RtspStagefright.patch           => r+ by sworkman 
part4-2.RtspStagefright.patch           => r+ by sworkman
part5.StreamingProtocolInterface.patch  => r+ by sworkman
part6.RtspControllerAndMetaData.patch   => r+ by jduell.mcbugs
Flags: needinfo?(sworkman)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(chris.double)
I talked to Vincent and these patches haven't changed significantly since the review by me/Steve/ted. So I think we're done with review of the code.

The issue now is that this code has no test infrastructure (Vincent has been testing manually against internet RTSP servers).  Vincent says the plan is to just land anyway for now and that we're going to need to write an RTSP server in the future for another reason ("wifi display feature") and we can then use it for automated tests as well.

Generally our policy is not to land major new features like protocols without automated tests.  I can imagine we might bend that rule for B2G if there's a compelling market reason.  I'm not sure who on the B2G team is available to make that case (Vincent, if you know, please needinfo? them).  Meanwhile I'm needinfo'ing :mcmanus since it will ultimately be his decision on whether to take this code w/o tests.
Flags: needinfo?(sworkman)
Flags: needinfo?(mcmanus)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(chris.double)
(In reply to Jason Duell (:jduell) from comment #205)
> I talked to Vincent and these patches haven't changed significantly since
> the review by me/Steve/ted. So I think we're done with review of the code.
> 
> The issue now is that this code has no test infrastructure (Vincent has been
> testing manually against internet RTSP servers).  Vincent says the plan is
> to just land anyway for now and that we're going to need to write an RTSP
> server in the future for another reason ("wifi display feature") and we can
> then use it for automated tests as well.

Note - that will only work if the servers run within release engineering infrastructure. Otherwise, that won't work, as buildbot-based automation is not allowed to hit anything outside of rel eng infrastructure realm per a security requirement from ops sec.

> 
> Generally our policy is not to land major new features like protocols
> without automated tests.  I can imagine we might bend that rule for B2G if
> there's a compelling market reason.  I'm not sure who on the B2G team is
> available to make that case (Vincent, if you know, please needinfo? them). 
> Meanwhile I'm needinfo'ing :mcmanus since it will ultimately be his decision
> on whether to take this code w/o tests.

FWIW - We don't really allow the market reason anymore now typically that we've moved to the train model on B2G. We've moved to a model now that argues more heavily for tests on landing to allow a feature to be marked as complete. This is due to the high technical debt we've collected now to the point that we're having difficulty maintaining existing feature sets as time passes, as automation has slipped through the cracks if it was not taken care of up front. Note that this model doesn't argue for extensive test coverage on landing - only at least the basics such that there's lightweight defect detection for obvious regressions on post feature work patches detected on landing.

Can we at least get something basic here to stop obvious regressions on patches post landing of this feature?
We have filed the Bug 877065 - "RTSP integration test" as our proposal for Rtsp test infrastructure. We need to setup a real Rtsp server(Darwin or live555 ...), and put a sample video on the server. The gaia app uses video tag to manipulate the Rtsp server, and compare the captured video. 

We may implement the Rtsp server as the next step, and eliminate the need for setup a real Rtsp server.
Attachment #798755 - Attachment is obsolete: true
Attachment #810947 - Flags: review?(chris.double)
Attachment #810948 - Flags: review?(roc)
Attachment #810949 - Flags: review?(roc)
Attachment #810950 - Flags: review?(roc)
(In reply to Vincent Chang[:vchang] from comment #208)
> Created attachment 810947 [details] [diff] [review]
> RtspResource.v10.patch

To make this easier/faster to review, what's this patch for? Is it updating for review comments? What changes have been made?
Flags: needinfo?(vchang)
(In reply to Chris Double (:doublec) from comment #212)
> (In reply to Vincent Chang[:vchang] from comment #208)
> > Created attachment 810947 [details] [diff] [review]
> > RtspResource.v10.patch
> 
> To make this easier/faster to review, what's this patch for? Is it updating
> for review comments? What changes have been made?

Hi Chris:
This patch address  the review comments you mentioned in comment 197. There is no logic change in the patch.
Thanks for review the patch.
Flags: needinfo?(vchang)
Flags: needinfo?(mcmanus)
Comment on attachment 810950 [details] [diff] [review]
RTSPURI_SCHEME.patch

Review of attachment 810950 [details] [diff] [review]:
-----------------------------------------------------------------

I am not a peer of this module.
Attachment #810950 - Flags: review?(roc) → review?(khuey)
Comment on attachment 810949 [details] [diff] [review]
Rtsp_moz.build.patch

Review of attachment 810949 [details] [diff] [review]:
-----------------------------------------------------------------

I am not a build-system peer
Attachment #810949 - Flags: review?(roc) → review?(gps)
Comment on attachment 810949 [details] [diff] [review]
Rtsp_moz.build.patch

Review of attachment 810949 [details] [diff] [review]:
-----------------------------------------------------------------

For future reference, this change counts as "trivial" and doesn't require build peer review. Generally speaking, the moz.build sandbox is tightly enough defined that we aren't worried about badness. Make files, however, we do care about because there are no restrictions there.
Attachment #810949 - Flags: review?(gps) → review+