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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel