[FFmpeg-devel] [PATCH] Add RTMFP support with librtmfp library

[Moritz Barsnick]

On Fri, Apr 19, 2019 at 19:20:50 +0200, Thomas Jammet wrote:
> @@ -6189,6 +6192,7 @@ enabled libopus           && {
>  enabled libpulse          && require_pkg_config libpulse libpulse
> pulse/pulseaudio.h pa_context_new
>  enabled librsvg           && require_pkg_config librsvg librsvg-2.0
> librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo

It looks like your mail client automatically wrapped the lines of the
patch, which will not allow it to be applied. You need to reconfigure
your mailer, or attach the patch (or use git send-email).

> + at item rtmfp_socketReceiveSize

CamelCase options aren't accepted by the project, AFAIU.

> +(Only with @code{rtmfp_negtroup}) Try to play an RTMFP unicast stream url
                    ^
You nicely misspelled this option reference six times (thanks to
copy/paste, likely). ;-)

> +(Only with @code{rtmfp_negtroup}) Specifies the maximum number (-1) of
> +peers to which we will send push fragments.

Does -1 mean no maximum? Or what does it mean here?

>  OBJS-$(CONFIG_LIBRTMPTE_PROTOCOL)        += librtmp.o
> +OBJS-$(CONFIG_LIBRTMFP_PROTOCOL)         += librtmfp.o
>  OBJS-$(CONFIG_LIBSMBCLIENT_PROTOCOL)     += libsmbclient.o

This is not quite the correct alphabetical order.

> +typedef struct LibRTMFPContext {
> +    const AVClass*      class;
> +    RTMFPConfig         rtmfp;
> +    unsigned int        id;
> +    int                 audioUnbuffered;
> +    int                 videoUnbuffered;
> +    int                 p2pPublishing;
> +    char*               peerId;
> +    char*               publication;
> +    unsigned short      streamId;
> +    const char*         swfUrl;
> +    const char*         app;
> +    const char*         pageUrl;
> +    const char*         flashVer;
> +    const char*         host;
> +    const char*         hostIPv6;

The asterisks attach to the variable, not the type.

And ffmpeg doesn't use CamelCase for regular variables.

> +static void onStatusEvent(const char* code, const char* description) {
> +  av_log(NULL, AV_LOG_INFO, "onStatusEvent : %s - %s\n", code, description);
> +}

This should get a log context.

> +    int res = 0;
> +
> +    res = RTMFP_Write(ctx->id, buf, size);

The first assignment is redundant.

> +    int res = 0;
> +
> +    res = RTMFP_Read(ctx->streamId, ctx->id, buf, size);

Same here.

> +    return (res < 0)? AVERROR_UNKNOWN : res;

Can you not find a better error code?

> +    {"audioUnbuffered", "Unbuffered audio mode (default to false)",
> OFFSET(audioUnbuffered), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1,
> DEC|ENC},
> +    {"videoUnbuffered", "Unbuffered video mode (default to false)",
> OFFSET(videoUnbuffered), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1,
> DEC|ENC},

These defaults usually don't need to be documented here (ffmpeg -h does
that for you).

> +    {"netgroup", "Publish/Play the stream into a NetGroup
> (multicast)", OFFSET(netgroup), AV_OPT_TYPE_STRING, {.str = NULL }, 0,
> 0, DEC|ENC},

The description makes it sound like a boolean. What does this parameter
actually expect?

> +    {"pushLimit", "Specifies the maximum number (-1) of peers to
> which we will send push fragments", OFFSET(pushLimit),
> AV_OPT_TYPE_INT, {.i64 = 4 }, 0, 255, DEC|ENC},

I still don't understand the "-1". And try to avoid the "we". (Yes, I
know passive form is sometimes considered weird in English.)

> +    {"updatePeriod", "Specifies the interval in milliseconds between
> messages sent to peers informating them that the local node has new
> p2p multicast media fragments available",

I believe the description should be shorter. (You can additionally save
some by dropping the "specifies", as each parameter specifies something
by nature.)

This was likely not complete, and no technical review though, sorry.

Cheers,
Moritz