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

[Maksym Veremeyenko]

On 24.07.2017 20:11, Nicolas George wrote:
> Le quintidi 5 thermidor, an CCXXV, Maksym Veremeyenko a écrit :
>> would it be ok:
>>
>>    Subject: [PATCH] lavf: implement NewTek NDI support
>
> Exactly. Except I mistyped: that would be lavd, not lavf; my bad.
fixed

>
>> would you *newtek_ndi* or *libndi_newtek"
>
> I prefer libndi_newtek, but that is only my advice.
>
fixed

>>> Is -vcodec wrapped_avframe really necessary? It should be automatic.
>> yes, i can remove that if you prefer.
>
> I think it is better if examples are as simple as possible.
>
fixed

>> as mentioned above, it used in a thread
>
> Yes, but you can give avctx to the thread and get the private context
> from it instead of giving the private context and getting avctx from it.
> I have not yet looked at what you did in the new version.
>
i dropped that

>>>> +        if (NDIlib_frame_type_video == t)
>>> The test looks strange in that direction.
>> why? that approach could save from mistypes like
>
> I know it is the usual justification for this idiom. But at the same
> time, it is anti-idiomatic for most people. We say "Alice is in her
> office", not "Alice's office contains her", and the same goes for
> comparisons. Furthermore, all decent compilers know to print a warning
> in these cases since quite some time.
>
> As above, if you intent to maintain this code on the long run, then
> decision is yours. Otherwise, better follow the usual coding style.
>
fixed

>>>> +            ndi_enqeue_video(ctx, &v);
>>>> +        else if (NDIlib_frame_type_audio == t)
>>>> +            ndi_enqeue_audio(ctx, &a);
>>>> +        else if (NDIlib_frame_type_metadata == t)
>>>> +            NDIlib_recv_free_metadata(ctx->recv, &m);
>>> Is there a risk of leak if a new type of packet is invented? Looks like
>>> a bad API design by NewTek.
>> API in a developing stage and not final - we can expect everythign
>
> You seem to be in contact with them, then you may report this issue: a
> function capable of freeing any kind of packet is absolutely necessary.
> Otherwise, if a new kind of packet is introduced, older applications
> will not be able to free them and leak. With a generic function,
> applications can just free any unknown packet.
>
i added unknow frame type warning

>> could you give me a hint/examples?
>
> I think it has been addressed in other mails.
>
i found it throw code and reworked completely

>> currently alpha channel is ignored
>
> Can you clarify who is ignoring the alpha channel?
>
alpha been ignored by *me* :-)
there is no appropriate pixel format for handling it.

>> i blindly copying further code from decklink module that provides setting
>> interlaced flag
>
> It is entirely possible that existing components have code that follow
> what is no longer considered best practice, especially when these
> components depend on proprietary libraries that few people have.
>
> On the other hand, new code is expected to follow the best practices. In
> particular, new code is expected to produce zero warnings with common
> compilers (gcc, clang).
>

in current implementation:

+    st->codecpar->field_order       = v->frame_format_type == 
NDIlib_frame_format_type_progressive
+        ? AV_FIELD_PROGRESSIVE : AV_FIELD_TT;


>> thread is reading, main app is writing only...
>
> This is a common misconception. The fact that the modification are
> unidirectional avoids race conditions, but they are not the only issue.
> The compiler is perfectly allowed to decide that this store is useless
> and optimize it away. Even if the compiler is prevented to do so using
> the "volatile" keyword, the processor can keep the value inside a cache
> that is not shared between SMP instances.
>
> For these reasons, a lock is not needed but a memory barrier is. A
> memory barrier is exactly the instruction to flush the data completely
> to/from shared memory.
>
i dropped thread for reading packet, so it is no more needed...


>>>> +    ctx->video->line_stride_in_bytes = avframe->linesize[0] < 0
>>>> +        ? -avframe->linesize[0]
>>>> +        : avframe->linesize[0];
>>> abs()?
>> copied from decklink code
>
> Could still be simplified in new code.
>


>>>> +    ctx->video->p_data = (avframe->linesize[0] < 0)
>>>> +        ? (void *)(avframe->data[0] + avframe->linesize[0] * (avframe->height - 1))
>>>> +        : (void *)(avframe->data[0]);
>>> Did you test with negative linesize? It looks like it will flip the
>>> video.
>> i did not test, but i leaved it for further processing
>
> I find this block quite suspicious. It really needs to be tested. But I
> do not know how to easily generate frames with negative strides.
>
i think it could work in synthetic tests or some TGA files has it or 
some real cases with OpenGL surface downloading...

>>> Check channel layout too.
>> i will drop this check at all
>
> Unfortunately, the documentation does not seem to be available without
> registration. Can you check what they tell exactly?
>
> If the library rejects channels configurations it does not support, then
> there is no need to check before.
>
> But if the library only checks the number of channels and makes
> assumptions about the layout (very common behaviour), then the calling
> code must check the layout. Otherwise, the auto-rematrixing code will
> not be triggered and the output will be wrong.
>
> Actually, did you check that they use the same channel order as FFmpeg ?
> Debugging problems with channel order is a real nightmare for users (I
> have spent hours myself on it when I purchased 5.1 speakers), so
> developers ought to be very careful about it.

documentation says:

[...]
supporting 4K (and beyond) along with 16 channels (and more) of 
floating-point audio.
[...]

so actually channels numbers not limited.

moreover, there is no channel layout - nor SDI, nor simple ADC/DAC

i attached latest version of patch, please review

-- 
Maksym Veremeyenko

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavd-implement-NewTek-NDI-muxer-demuxer-support.patch
Type: text/x-patch
Size: 32460 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170726/5518cc99/attachment.bin>