[FFmpeg-devel] [PATCH] Implement NewTek NDI support
verem at m1stereo.tv
Wed Jul 26 10:39:18 EEST 2017
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.
>> would you *newtek_ndi* or *libndi_newtek"
> I prefer libndi_newtek, but that is only my advice.
>>> 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.
>> 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.
>>>> + 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 ==
+ ? 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
>>>> + ? -avframe->linesize
>>>> + : avframe->linesize;
>> copied from decklink code
> Could still be simplified in new code.
>>>> + ctx->video->p_data = (avframe->linesize < 0)
>>>> + ? (void *)(avframe->data + avframe->linesize * (avframe->height - 1))
>>>> + : (void *)(avframe->data);
>>> Did you test with negative linesize? It looks like it will flip the
>> 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.
supporting 4K (and beyond) along with 16 channels (and more) of
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
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 32460 bytes
Desc: not available
More information about the ffmpeg-devel