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

[Nicolas George]

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 still have some doubts about stream time_base used, i think it should be
> 1/10000000 like their timecode value, rather then framerate, because if
> source will provide variable frame rate it would be hard to compute real
> frame duration

If the protocol supports VFR, then yes, keeping the timestamps is
probably the best choice. Actually, if they decided to provide these
timestamps, there is no reason to rescale them here, be it for video or
for audio.

> i can use __func__ but if you prefer, i will drop this.

My personal preference would be a message that can easily be found with
"git grep". But if you intend to maintain this code on the long run, the
decision is yours.

> >>+        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.

> could you give me a hint/examples?

I think it has been addressed in other mails.

> currently alpha channel is ignored

Can you clarify who is ignoring the alpha channel?

> 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).

> 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.

>						 but if you give me a example
> from ffmpeg's code or more appropriate approach for notifying thread to
> exit, i will reimplement it

The simplest way of getting a memory barrier is a mutex, of course, even
though it is a bit overkill. Modern C has features in stdatomic.h that
can probably be of use, but I do not know how to use them. Also,
AVThreadMessageQueue has the feature of sending a single integer code
from reader to writer.

And of course, if you get rid of the thread, all this is moot.

> >>+    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.

> >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.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170724/d45d07a9/attachment.sig>