[FFmpeg-devel] [PATCH] RTMPE support

[Howard Chu]

Stefano Sabatini wrote:
> On date Saturday 2010-03-27 00:13:26 -0700, Howard Chu encoded:
>> Howard Chu wrote:
>>> Kostya wrote:
>>>> On Thu, Mar 25, 2010 at 09:06:51PM -0700, Howard Chu wrote:
>>>>> +fail:
>>>>> +    av_free(host);
>>>>> +    av_free(playpath.av_val);
>>>>
>>>> Where are those allocated? And have you tested this path?
>>>
>>> In RTMP_ParseURL. Yes, this part is ugly, could be done differently. And yes,
>>> it works here.
>>
>> I've moved the option parsing into librtmp, so none of these values
>> are exposed now. This is better in the long run since RTMP is still
>> a moving target and we're still revving new stuff into librtmp
>> pretty frequently.
> [...]
>
> So if I understood correctly the various "swfurl", "pageurl", etc
> parameters are directly specified in the filename?

> Can you say how they need to be specified?

Yes, like the example at the bottom of this message:

http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2010-March/063988.html

Values containing spaces will need to be URLencoded.

Compare the set of options I originally implemented here 
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2010-March/085026.html
(just 8 keywords)

with the complete set in librtmp here
http://lists.mplayerhq.hu/pipermail/rtmpdump/2010-March/000708.html
(17 keywords)

>> +#include "libavcodec/opt.h"
>
> Not anymore required.

OK, fixed.

>> +    switch(av_log_get_level()) {
>> +    default:
>> +    case AV_LOG_FATAL:
>> +        rc = RTMP_LOGCRIT; break;
>> +    case AV_LOG_ERROR:
>> +        rc = RTMP_LOGERROR; break;
>> +    case AV_LOG_WARNING:
>> +        rc = RTMP_LOGWARNING; break;
>> +    case AV_LOG_INFO:
>> +        rc = RTMP_LOGINFO; break;
>> +    case AV_LOG_VERBOSE:
>> +        rc = RTMP_LOGDEBUG; break;
>> +    case AV_LOG_DEBUG:
>> +        rc = RTMP_LOGDEBUG2; break;
>
> These can be put on one line and vertically aligned, should help
> readability.

OK.

>> +static int rtmp_read(URLContext * s, uint8_t * buf, int size)
>
> Nit: please use consistently TYPE *VAR rather than TYPE * VAR, here
> and below.

OK. (cruft from running indent without specifying all typedefs...)

>> +static int64_t rtmp_seek(URLContext * s, int stream_index,
>> +                         int64_t timestamp, int flags)
>> +{
>> +    RTMP *r = s->priv_data;
>> +
>> +    if (flags&  AVSEEK_FLAG_BYTE)
>> +        return AVERROR(ENOTSUP);
>
> As I'm now an expert of error codes, I can say this is not very
> correct:
> http://www.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html
> [ENOTSUP]
>      Not supported. The implementation does not support this feature of the Realtime Option Group.
>
> Please use AVERROR_NOTSUPP instead (but there is still discussion
> about the real meaning of that error code).

OK.

>> +    /* seeks are in milliseconds */
>> +    timestamp = av_rescale(timestamp, AV_TIME_BASE, 1000);
>> +    if (!RTMP_SendSeek(r, timestamp))
>> +        return -1;
>> +    return timestamp;
>> +}
>> +
>> +URLProtocol rtmp_protocol = {
>> +    "rtmp",
>> +    rtmp_open,
>> +    rtmp_read,
>> +    rtmp_write,             /* write */
>> +    NULL,                   /* seek */
>> +    rtmp_close,
>> +    NULL,                   /* next */
>> +    rtmp_pause,
>> +    rtmp_seek
>> +};
>
> The " /* write */" here and below is slightly confusing, as it is
> indeed implemented, while the "/* seek */" and "/* next */" are
> evidently useful for indicating which are the not
> implemented/implementable features.

I hadn't implemented write in the first version of the patch, and just left 
the comment after implementing it. Anyway, cleaned up now.

Along those lines I have a question - who uses the get_file_handle() hook? 
Presumably the only safe thing to do with it is with select() or some other 
event loop code. Is it important to implement this? I haven't found any code 
that uses it. (I've added it this time around.)
>
> Thanks for your work, regards.

Thanks for the review.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dif2.txt
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100327/264a7438/attachment.txt>