[FFmpeg-devel] RTMPE support

[Howard Chu]

Kostya wrote:
> On Mon, Mar 15, 2010 at 08:48:23AM -0700, Howard Chu wrote:
>> Carl Eugen Hoyos wrote:
>>> Howard Chu<hyc<at>   highlandsun.com>   writes:
>>>
>>>> Here's a librtmp wrapper for libavformat. These diffs are needed as well:
>>>
>>> The preferred method to post such a change is:
>>> svn add libavformat/rtmp2.c
>>> svn di libavformat>patch.diff
>>
>> Thanks. This code was not a candidate for inclusion, there are still
>> outstanding problems that need to be resolved first, as I mentioned in my
>> previous message. This is just to get the conversation going again.
>>
>>> I don't think the patch should disable the current code (did you open
>>> bug-reports for the cases where it does not work?) and your new file contains
>>> tabs that cannot be committed to svn (please consider running tools/patcheck).
>>
>>> Before sending a new patch, the usefulness of adding a new external dependency
>>> should probably be discussed.
>>
>> I didn't think a bug report about RTMPE was necessary, it's obvious that
>> the existing code doesn't support it. I agree that this area needs to be
>> discussed; compn raised the question already
>>
>> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-September/075579.html
>>
>> but the conversation died. Re: external dependencies - if you want to
>> have RTMPE support, you're going to need to add a crypto library. As I
>> already pointed out in my previous email, if you're going to add a crypto
>> library, you can also get https at the same time.
>>
>> I mentioned a few problems in the existing code to kostya on
>> #ffmpeg-devel a few hours ago but will summarize more extensively here:
>>
>> The existing handshake code has a problem, it hangs when trying to
>> connect to an rtmplite server. It also only checks in one place for the
>> handshake digest, while in practice there are two different locations
>> that need to be checked, depending on the server version. It appears to
>> me that this code has not been tested against enough RTMP server
>> implementations.
>
> I agree it has not been tested against many existing RTMP server
> implementations for various reasons.
>
> But some of your claims regarding my code are false. For example, it
> checks in two places for digest (and has works with plain old
> handshake).

Sorry if I misread or misrepresented anything. It's also possible I'm not 
seeing the latest code, I was working with the mplayer svn.

>> The existing code doesn't implement Pause or Seek. To me, one of the main
>> reasons to even bother with RTMP support inside the player is to have
>> Seek support, otherwise we can just use rtmpdump | mplayer and get linear
>> playback.
>
> Mostly because there's no easy way to hint protocol handler to send
> pause. And protocol handler seek takes file offset and not timestamp, so
> it's unknown how to translate one into another.

I added the url_read_pause entry point and the protocol handler used it, so 
that didn't appear to be a real stumbling block. I also added the 
url_read_seek entry point which is time based, but that apparently had no effect.

>> The existing code only implements the FlashPlayer 9 handshake. Some sites
>> (like Hulu) are now configured to require FlashPlayer 10, and will reject
>> otherwise valid connection attempts that use FP9.
>
> /me does not live in USA so no Hulu here.
> But FP10 handshake can be added.

>> The existing code doesn't implement RTMPE, doesn't implement RTMPT, and
>> doesn't implement SWF Verification. (Obviously; that's the original point
>> of this Subject.) These features could of course be added to it, but to
>> me that's wasteful duplication of effort.
>
> I agree with that (unless someone wants yet another SSL implementation
> in FFmpeg ;).
>
>> The existing code doesn't support audio-only streams. It generates a
>> hard-coded header indicating audio+video content, rather than basing the
>> header on the actual stream content.

> Can be fixed. And I've tested it, seemed to work on audio-only streams
> fine.

OK. Since I wasn't able to run this code against my test RTMP servers, I 
couldn't verify. I pointed it out because older versions of rtmpdump did the 
same, and we had a couple complaints from web-radio listeners.

>> The existing code doesn't support FMS 3.5 buffer management. FMS 3.5 will
>> stop sending data arbitrarily, independent of the RTMP buffertime that
>> was negotiated. Without being prodded back into resuming, the connection
>> will simply close due to inactivity, leaving you with an incomplete
>> stream.
>>
>> The existing code doesn't generate BytesRead reports. Some servers will
>> close the connection if these reports are not received at the expected
>> intervals.
>
> Huh? I've added sending "bytes read" report about a month ago.

Then my copy is out of date. Sorry for that.

>> The existing code uses a single chunk_size setting, but RTMP chunk size
>> needs two settings, since the server->client and client->server sizes are
>> independent. As it is, when a server sends a larger chunk size to the
>> client (as it always does, when playing media) it will cause the wrong
>> chunk size to be used for subsequent client->server packets.
>
> Can be fixed.
>
>> The existing code doesn't handle several responses that Adobe FMS
>> routinely generates. I'm not sure how critical this is, since I haven't
>> seen this code work yet, but at least it fails to duplicate the behavior
>> of an actual Flash client.
>
> The zeroeth question is should it do so.

Right, as I said, not suure how critical it is. But responding to the 
bandwidth check would probably be a good thing, as it lets the server make 
smarter decisions about how to allocate its resources.

>> Aside from feature completeness and correctness, I have some other doubts
>> about this code. The ff_amf_write_*() functions don't do any bounds
>> checking; it's possible (however unlikely) for gen_connect() to cause a
>> buffer overrun. There are other basic inefficiencies and such but there's
>> obviously more significant things to worry about first.
>
> It relies on knowledge how much bytes it takes to write data, so buffer
> is allocated with knowledge of output data size.

Yes, in every other case I see the size being calculated to fit the data. 
gen_connect is the only one I saw that used a fixed size. Since you're using 
4KB there, it is pretty unlikely for this to be a problem. But at the same 
time, working with sites like Hulu and others that use ridiculously long URLs 
and auth strings, it is *possible* for this to be overrun.

> And I admit that I'm lazy and have little interest in RTMP. I try to
> maintain it though.
>
> Yet I'd like to hear from major contributors whether we need native RTMP
> support or can ditch it for rtmpdump interface. Vad tror du? ?? ???????
> ? ????? ????????

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/