[FFmpeg-devel] [PATCH] Implement NewTek NDI support

[Maksym Veremeyenko]

On 23.07.2017 14:53, Marton Balint wrote:
[...]
>> @@ -3012,6 +3014,10 @@ decklink_indev_deps="decklink threads"
>>  decklink_indev_extralibs="-lstdc++"
>>  decklink_outdev_deps="decklink threads"
>>  decklink_outdev_extralibs="-lstdc++"
>> +ndi_indev_deps="ndi pthreads"
>
> you only need threads not pthreads.
>
ok

>> +ndi_indev_extralibs="-lndi"
>> +ndi_outdev_deps="ndi pthreads"
>
> as far as I see you don't need any threading here.
>
yes

>> + at item find_sources
>
> Maybe list_devices would be a better name, it is more consistent with
> other devices.
>
according to SDK documentation terminology, *source* and *find* used for 
this, there are no actual *devices* but only network sources.

>> + at item allow_video_fields
>> +When this flag is @option{false}, all video that you receive will be
>> progressive.
>> +Defaults to @option{1}.
>
> Defaults to @option{true}.
>
yes


>> +typedef struct AVPacketQueue {
>> +    AVPacketList *first_pkt, *last_pkt;
>> +    int nb_packets;
>> +    unsigned long long size;
>> +    int abort_request;
>> +    pthread_mutex_t mutex;
>> +    pthread_cond_t cond;
>> +    AVFormatContext *avctx;
>> +} AVPacketQueue;
>
> I guess the AVPacketQueue is duplicated from decklink_dec. So you either
exactly

> factorize that into a seprarate file, or you may also consider not using a
> queue here, and get rid of it, because according to the SDK, NDI lib
> also has a
> small (how small?) internal queue, so a few frames of buffering is
> provided by
> the NDI lib itself, therefore doing it on your own might not be needed
> at all.
> If you do this, you can drop the threads dependancy here as well.
>
it has internal queue, but the main goal of using AVPacketQueue is to 
avoid loosing packets during *probe* operation, if we discard this 
(ignore that packets or i find how deal in a case of AVFMTCTX_NOHEADER) 
then all decklink's AVPacketQueue code could be dropped



>> +    av_log(avctx, AV_LOG_INFO, "Found %d NDI sources:\n", n);
>> +    for (i = 0; i < n; i++)
>> +    {
>> +        av_log(avctx, AV_LOG_INFO, "\t'%s'\t'%s'\n",
>> ndi_srcs[i].p_ndi_name, ndi_srcs[i].p_ip_address);
>> +        if (!strcmp(name, ndi_srcs[i].p_ndi_name)) {
>> +            *source_to_connect_to = ndi_srcs[i];
>> +            j = i;
>> +        }
>> +    }
>
> You might only want to list all sources, if the user wanted
> list_sources. If
> the user chose a specific source, I don't think there is a good reason
> to list
> all available sources.
>
we have to find all sources to operate with a user's submitted value, 
because creating receiver operates with a struct returned by 
find_sources function rather then a name.

i can suppress printing found sources until find_sources specified


>> +    /* Find available sources. */
>> +    ret = ndi_find_sources(avctx, avctx->filename,
>> &recv_create_desc.source_to_connect_to);
>> +    if (ctx->find_sources) {
>> +        return AVERROR_EXIT;
>> +    }
>> +    if (ret < 0) {
>> +        return AVERROR(ENOENT);
>
> if (ret < 0)
>     return ret;
>
yes


-- 
Maksym Veremeyenko