[FFmpeg-devel] Implement NewTek NDI support

On 30.07.2017 17:19, Marton Balint wrote:
[...]
>> Subject: [PATCH] lavd: implement NewTek NDI muxer/demuxer support
>
> lavd: implement NewTek NDI input/output device support
>
fixed

[...]
>>  - Dolby E decoder and SMPTE 337M demuxer
>> +- NewTek NDI muxer/demuxer
>
> NewTek NDI input/output device
>
fixed

[...]
>> +@item wait_sources
>> +Override time to wait until the number of online sources have changed.
>> +Defaults to @option{500000}.
>
> Defaults to @option{0.5} sec.
>
> By the way, is this enough? SDK warns that it might take a few seconds
> to list all the sources. SDK example uses 5 second, shouldn't we us that
> as well as the default?
>
we can increase it after... but from my tests it is enough. moreover, i 
did some rework of /waiting process/

>> +@item max_frames_probe
>> +Maximum frames for probe input format.
>> +Defaults to @option{25}.
>
> This option is no longer needed/used.
>
fixed

[...]
>> +NDI is very picky about the formats it supports. Pixel format is always
>> +uyvy422. Audio sample rate is always 48 kHz.
>
> Is there a real limitation to 48 kHz? You might support anything unless
> there
> is a good reason to restrict it to 48 kHz.
>
no limitation:

[...]
sample_rate (int)
This is the current audio sample rate. For instance, this might be 
44100, 48000 or 96000. It can, however, be any value.
[...]

i will drop any checks of sample rate

>> +    int max_frames_probe;
>
> no longer needed.
>
done

[...]
>> +    const NDIlib_find_create_t find_create_desc = { true, NULL };
>
> for readability you should name your set fields, e.g. find_create_desc =
> { .show_local_sources = true };
>
done

[...]
>> +    NDIlib_find_destroy(ndi_find);
>
> Probably you should not destroy this here, only in ndi_read_close after
> freeing
> ctx->recv, because as far as I see the found NDIlib_source_t is might be
> freed
> here as well which you would also like to use after returning from this
> function.
>
i moved ndi_find to context and destroying it moved to ndi_read_close

[...]
>> +    const NDIlib_tally_t tally_state = { true, false };
>
> for readability you should name your set fields
>
done

[...]
>> +    st->time_base                   = NDI_TIME_BASE_Q;
>> +    av_stream_set_r_frame_rate(st, av_make_q(v->frame_rate_N,
>> v->frame_rate_D));
>> +
>> +    st->sample_aspect_ratio = st->codecpar->sample_aspect_ratio =
>> +        av_mul_q(av_d2q(v->picture_aspect_ratio, INT_MAX),
>> (AVRational){v->yres, v->xres});
>
> Since the source aspect ratio is float, and there are some very inaccurate
> representations of 16/9 in the include files and sdk (e.g. 16/9 = 1.778),
> I suggest we drop some precision here as well because we don't want to get
> 134217729:134217728 kind of sample aspect ratios.
>
> Something like:
>
> tmp = av_mul_q(av_d2q(v->picture_aspect_ratio, INT_MAX),
> (AVRational){v->yres, v->xres});
> av_reduce(&st->sample_aspect_ratio.num, &st->sample_aspect_ratio.den,
> tmp.num, tmp.den, 1000);
>
done

[...]
>> +
>> +    st->codecpar->codec_type        = AVMEDIA_TYPE_VIDEO;
>> +    st->codecpar->width             = v->xres;
>> +    st->codecpar->height            = v->yres;
>> +    st->codecpar->codec_id          = AV_CODEC_ID_RAWVIDEO;
>> +    st->codecpar->bit_rate          = av_rescale(v->xres * v->yres *
>> 16, st->time_base.den, st->time_base.num);
>
> Here you probably want to use frame rate instead of time base.
>
fixed

>> +//    avpriv_set_pts_info(st, 64, 1, a->sample_rate);
>
> stale line which can be removed
>
done

[...]
>> +    if (ctx->ndi_send) {
>> +        if (ctx->last_avframe) {
>> +            /* send NULL to flush equeued frame */
>> +            NDIlib_send_send_video_async(ctx->ndi_send, NULL);
>
> This line crashes for me under x86_64 linux, it seems to dereference the
> null
> pointer. SDK bug?
yes, it is SDK (libndi.so.1.0.1) bug. SDK provided by request (seems 
betta, libndi.so.3.0.1) has no this bug.

> Maybe you should simply do:
>
>          NDIlib_send_destroy(ctx->ndi_send);
>          av_frame_free(&ctx->last_avframe);
>
> Although I don't know if this flushes (sends) the last frame, or simply
> drops
> it... Anyhow, better than crashing, if you can't fix the crash.
>
done

[...]
>> +        : (void *)(avframe->data[0]);
>
> As NDI has no way of signalling a flipped image, I think it is best if you
> simply reject frames with negative linesize.
>
may be warning will be enough?

>> +    if (c->sample_rate != 48000) {
>> +        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
>> +               " Only 48kHz is supported.\n");
>> +        return AVERROR(EINVAL);
>> +    }
>
> As I mentioned earlier, you should consider supporting any sample rate.
>
done

>> +
>> +    ctx->audio =
>> av_mallocz(sizeof(NDIlib_audio_frame_interleaved_16s_t));
>> +    if (!ctx->audio)
>> +        return AVERROR(ENOMEM);
>> +
>> +    ctx->audio->sample_rate = c->sample_rate;
>> +    ctx->audio->no_channels = c->channels;
>> +    ctx->audio->reference_level = ctx->reference_level;
>> +
>> +    avpriv_set_pts_info(st, 64, 1, NDI_TIME_BASE/*c->sample_rate*/);
>
> stale comment
>
cleaned

[...]
>> +    ctx->video->xres = c->width;
>> +    ctx->video->yres = c->height;
>> +    ctx->video->frame_rate_N = st->avg_frame_rate.num;
>> +    ctx->video->frame_rate_D = st->avg_frame_rate.den;
>> +    ctx->video->frame_format_type = c->field_order ==
>> AV_FIELD_PROGRESSIVE
>> +        ? NDIlib_frame_format_type_progressive
>> +        : NDIlib_frame_format_type_interleaved;
>
> You should reject bottom field first formats, as NDI explicitly
> disallows them.
>
even if it disallowed, why to drop? may be warning message would be enough?

[...]
>> +//    avpriv_set_pts_info(st, 64, st->time_base.num, st->time_base.den);
>
> please remove this stale line as well.
>
done

>> +    avpriv_set_pts_info(st, 64, 1, NDI_TIME_BASE);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ndi_write_header(AVFormatContext *avctx)
>> +{
>> +    int ret = 0;
>> +    unsigned int n;
>> +    struct NDIContext *ctx = avctx->priv_data;
>> +    const NDIlib_send_create_t ndi_send_desc = {avctx->filename,
>> NULL, true, false};
>
> for readability you should name your set fields here as well.
>
done

> You might consider adding connection metadata to identify libavdevice as
> sender
> and signal the version. You might use something like "Lavd
> libndi_newtek" for
> the product name and use LIBAVDEVICE_VERSION for the version number.
>
i planned to add this later with user's defined metadata.

> I tried receiving an NDI source I created with ffmpeg, but it was not
> listed.
> Is this expected? I thought local sources should appear as well.
>
it should... that is my current testing environment
i reworked *ndi_find_sources*, please do a tests

> Also, v3 of the SDK is to be released very soon. Maybe better to wait
> for it
> and use that? Hopefully less bugs in the SDK itself?
i hope they will release it till IBC... but it has very small difference 
against current patch to support this.

[FFmpeg-devel] Implement NewTek NDI support

Commit Message

Comments

Patch