[FFmpeg-devel] [PATCH] RTMPE support

[Stefano Sabatini]

On date Saturday 2010-03-27 23:28:44 -0700, Howard Chu encoded:
> 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.
[...]

OKed by Kostya on IRC and applied.
-- 
FFmpeg = Fabulous Funny Most Portable Elitarian Gadget