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

[Marton Balint]

On Sun, 23 Jul 2017, Nicolas George wrote:

[..]

>> +    int abort_request;
>> +    pthread_mutex_t mutex;
>> +    pthread_cond_t cond;
>> +    AVFormatContext *avctx;
>> +} AVPacketQueue;
>
> This whole API looks duplicated in the encoder. Not acceptable. You need
> to move it into a pair of .c/.h files, with the "ff_" prefix for the
> function names (the structure name can stay AV, since it is not
> exported).
>
> But I think it would be better if you tried to use AVThreadMessageQueue
> instead. You can extend it if necessary.

That is copied from decklink_dec as far as I see. Maybe better to 
factorize first, and move it to AVThreadMessageQueue as a second step?



>> +    pkt1 = (AVPacketList *)av_malloc(sizeof(AVPacketList));
>
> Drop the cast, it is useless in C (this is not c++) and harmful (it can
> hide warnings).

I guess some of the casts are there because decklink_dec was c++ code...



>> +    /* Probe input */
>> +    for (ret = 0, i = 0; i < ctx->max_frames_probe; i++)
>
> This is already implemented, in a more generic and uniform way, in
> libavformat. Drop the loop and move the format-probing code into
> ndi_read_packet().

Maybe it's just me, but I am not sure what kind of probing are you 
referring to. Could you explain in a bit more detail how the changed code 
should work?



>> +    AVFrame *frame = ctx->video_st ? ctx->video_st->codec->coded_frame : NULL;
>
> This line produces a warning, does it not? Anyway, you are not supposed
> to use st->codec any longer.

I guess that is copied from decklink as well. As far as I see, the correct 
replacement is to set codecpar->field_order in read_header, and that is 
it, right?



>> +    { "wait_sources", "Time to wait until the number of online sources have changed (ms)"  , OFFSET(wait_sources), AV_OPT_TYPE_INT, { .i64 = 500 }, 100, 10000, DEC },
>
> AV_OPT_TYPE_DURATION

Note: duration is microsec, not milisec, so code/docs needs updating as 
well.


>> +    }
>> +    if (c->channels != 2 && c->channels != 4 && c->channels != 8) {
>> +        av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
>> +               " Only stereo and 7.1 are supported.\n");
>
> Check channel layout too.

I think it is better if it works with unkown channel layouts as well, as 
long as the number of channels are supported.



>> +    if (ctx->video) {
>> +        av_log(avctx, AV_LOG_ERROR, "Only one video stream is supported!\n");
>
>> +        return AVERROR(ENOTSUP);
>
> I suspect this library exists also for Windows and Macos, so you cannot
> use ENOTSUP. EINVAL.

Or maybe ENOSYS?


Regards,
Marton