On 08/03/2016 03:41 PM, Nicolas George wrote:
Le sextidi 16 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :

+    if (ctx->termination_requested)
+        return 1;
This is a common misconception: volatile is not enough for thread
communication. IIUC, volatile forces the compiler to issue actual store and
fetch operations to memory, but not a memory barrier. That means it can be
used for communication with a signal handler (provided the object is small
enough so that operations are atomic, cf. sig_atomic_t) or a hardware
device. But for inter-thread communication on SMP systems, the lack of
memory barrier means that the change may be limited to the processor's cache
and only reach the central shared memory and the other core's cache at some
indeterminate point later.

For inter-thread communication, you either need a mutex or a special atomic
and synchronized operation. The second solution is more efficient, but
currently less portable. You can have a look at libavutil/atomic.h, and also
at Anton's efforts to replace it with a more standard and modern API, it
happened just last week on the fork's mailing-list.
This was kind of intentional, since the flag is uint8_t and only thing what happens is that it is set to 1(from initial 0) at some point of time I think the worst scenario is that flag will be detected later (but that also happens when the lock is used), there is no risk that corrupted value will be read. But I agree this is probably not a good practice, so I will change the flag to be int and use atomic get/set.
+
+    return ff_check_interrupt(&ctx->orig_interrupt_callback);
I am not sure if this part is worth your efforts: if the application wants
to cancel a write, it uses the FIFO muxer APIs, it does not need to set up
an interrupt callback. Do you see a case where it would be useful?
I think there is no reason to disable custom interrupt callback here, there is almost no overhead if it is used, user can choose to ignore it, but when he decides to use it it will work as expected.
+        ret = pthread_tryjoin_np( fifo->writer_thread, NULL);
This function is a GNU extension (np means nonportable), this is the reason
for the build failure reported by Michael; I am rather surprised you did not
get it too since the build options should hide nonportable functions.

Anyway, this functions is rather unnecessary, you can just set a flag just
before the thread terminates and check the flag before pthread_join().
Yes, this was a mistake, I've changed that to use flag.

Thanks!

Regards,
Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to