Quoting Julian Seward (2016-08-01 12:56:03) > Hi Anton, > > Nice to see your work on race/UB removal. > > I have been working on removing races from libavcodec/pthread_frame.c > as part of a wider effort to remove TSan-detected races from Firefox, > which uses ffmpeg-3.0.2. This is tracked starting at > https://bugzilla.mozilla.org/show_bug.cgi?id=1274256#c34. > > Your fixes mostly look good and necessary to me. However, for patches > 10 and 11 of your series, I had a different approach, and I am > wondering if you would consider using it instead. The proposed patch > is at the end of this message. > > > [PATCH 10/13] pthread_frame: use atomics for signalling threads to die > > I believe that changing fctx->die to an atomic_int makes TSan quiet but > does not really remove the race, which still exists at a higher level > of granularity. The store to fctx->die should be atomic, yes, but I > rewrote the reading side of it like this: > > pthread_mutex_lock(&p->mutex); > while (1) { > - while (p->state == STATE_INPUT_READY && !fctx->die) > - pthread_cond_wait(&p->input_cond, &p->mutex); > + while (p->state == STATE_INPUT_READY) { > + pthread_cond_wait(&p->input_cond, &p->mutex); > + if (fctx->die) break; > + } > > if (fctx->die) break; > > This is for ffmpeg 3.0.2, slightly different from trunk, but the idea > is the same: move the read of fctx->die so that it is guarded by p->mutex.
Sure, I guess that also works. I have no problem replacing my patch with yours, if you can send that as a git-formatted separate change. > > > [PATCH 11/13] pthread_frame: use atomics for PerThreadContext.state > > There are two instances of double-checked locking on > PerThreadContext::state, one in submit_packet, one in > ff_thread_decode_frame. Per analysis at > https://bugzilla.mozilla.org/show_bug.cgi?id=1274256#c40 I believe the > double-checked locking saves 3 global memory operations per decoded > frame and so is pointless as an optimisation. > > I don't think it is desirable to make ::state atomic, for the same > reason as mentioned above for fctx->die. Instead I prefer to fix > races using the pthread_* functions wherever possible. I think this is a bit more tricky here, since there is no single mutex that guards state modifications in all cases. -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
