[FFmpeg-devel] [PATCH] Implement NewTek NDI support
Maksym Veremeyenko
verem at m1stereo.tv
Thu Aug 3 08:21:09 EEST 2017
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
[...]
>> + at 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/
>> + at 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.
--
Maksym Veremeyenko
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavd-implement-NewTek-NDI-input-output-device-suppor.patch
Type: text/x-patch
Size: 32639 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170803/9877ab66/attachment.bin>
More information about the ffmpeg-devel
mailing list