Vincent Chang[:vchang][changyihsin] Assignee
	Description • 12 years ago

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.

	Kai-Chih Hu [:khu]
	Comment 1 • 12 years ago

Reference: bug 506834.

	Kai-Chih Hu [:khu]
	Comment 2 • 12 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 12 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 4 • 12 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 12 years ago

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

	Benjamin Chen [:bechen]
	Comment 6 • 12 years ago

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);|

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 12 years ago

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 Chang[:vchang][changyihsin] Assignee
	Comment 8 • 12 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 9 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 10 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 11 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 12 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 13 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 14 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 15 • 12 years ago

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.

	Jason Duell
	Comment 16 • 12 years ago

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.

	Jason Duell
	Comment 17 • 12 years ago

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.

	Jason Duell
	Comment 18 • 12 years ago

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.

	Benjamin Chen [:bechen]
	Comment 19 • 12 years ago

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)

	Benjamin Chen [:bechen]
	Comment 20 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 21 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 22 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 23 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 24 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 25 • 12 years ago

refactor the patch due to new file moz.build

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 26 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 27 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 29 • 12 years ago

This patch includes the IPC code and new interfaces.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 30 • 12 years ago

compiler flag for RTSP

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 33 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 34 • 12 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 35 • 12 years ago

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)

	u459114
	Comment 36 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 37 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 38 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 39 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 40 • 12 years ago

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;

	Steve Workman [:sworkman] (INACTIVE)
	Comment 41 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 42 • 12 years ago

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?

	Steve Workman [:sworkman] (INACTIVE)
	Comment 43 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 44 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 45 • 12 years ago

> > +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*

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 46 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 48 • 12 years ago

Address the feedback comments and fix bugs.

	Benjamin Chen [:bechen]
	Comment 49 • 12 years ago

fix bug and add some somments

	Benjamin Chen [:bechen]
	Comment 50 • 12 years ago

bug fix

	cajbir (:cajbir)
	Comment 51 • 12 years ago

Why is this blocked by bug 856542?

	Benjamin Chen [:bechen]
	Comment 52 • 12 years ago

(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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 54 • 12 years ago

(In reply to Vincent Chang[:vchang] from comment #53)
> Created attachment 737361 [details]
> Draft architecture v0.1

Thanks for that - that helps my understanding.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 55 • 12 years ago

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.

	Jason Duell
	Comment 56 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 57 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 58 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 59 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 60 • 12 years ago

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

	Jason Duell
	Comment 61 • 12 years ago

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.

	Jason Duell
	Comment 62 • 12 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 63 • 12 years ago

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

	Jason Duell
	Comment 64 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 65 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 66 • 12 years ago

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 ??)

	Thinker Li [:sinker]
	Comment 67 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 68 • 12 years ago

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

	Jason Duell
	Comment 69 • 12 years ago

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.

	Thinker Li [:sinker]
	Comment 70 • 12 years ago

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.

	Jason Duell
	Comment 71 • 12 years ago

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

	Jason Duell
	Comment 72 • 12 years ago

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!

	Steve Workman [:sworkman] (INACTIVE)
	Comment 73 • 12 years ago

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.

	Benjamin Chen [:bechen]
	Comment 74 • 12 years ago

Support the live stream.
RTSPTrackBuffer bug fix.

	Benjamin Chen [:bechen]
	Comment 75 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 77 • 12 years ago

1. Switch the capitalization from RTSP to Rtsp

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 78 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 79 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 80 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 81 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 82 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 83 • 12 years ago

1. Switch the capitalization from RTSP to Rtsp

	(not currently active) Ted Mielczarek
	Comment 84 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 85 • 12 years ago

Address the review comment.

	(not currently active) Ted Mielczarek
	Comment 86 • 12 years ago

Comment on attachment 747763 [details] [diff] [review]
1. Patch v04 - Add MOZ_RTSP compiler flag

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

Thanks!

	Steve Workman [:sworkman] (INACTIVE)
	Comment 87 • 12 years ago

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?

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 88 • 12 years ago

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

	Steve Workman [:sworkman] (INACTIVE)
	Comment 89 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 90 • 12 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 91 • 12 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 93 • 12 years ago

rebase and address the try server errors

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 94 • 12 years ago

rebase and address try server errors.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 96 • 12 years ago

rebase and address try server errors

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 97 • 12 years ago

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

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 98 • 12 years ago

rebase and address trying server error.

	Benjamin Chen [:bechen]
	Comment 99 • 12 years ago

(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?

	Benjamin Chen [:bechen]
	Comment 100 • 12 years ago

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

	Jason Duell
	Comment 101 • 11 years ago

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?

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 102 • 11 years ago

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.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 103 • 11 years ago

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.