[FFmpeg-devel] [PATCH] pthread_frame: make accesses to debug field be protected by owner lock.

[Wan-Teh Chang]

Hi Ronald,

On Wed, Jul 5, 2017 at 7:24 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi Wan-Teh,
>
> On Wed, Jul 5, 2017 at 8:08 PM, Wan-Teh Chang <wtc at google.com> wrote:
>>
>> Hi Ronald,
>>
>> On Wed, Jul 5, 2017 at 3:31 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi Wan-Teh,
>> >
>> > On Wed, Jul 5, 2017 at 6:12 PM, Wan-Teh Chang <wtc at google.com> wrote:
>> >>
>> >> Thank you for all the tsan warning fixes. In the meantime, it would be
>> >> good to revert 2e664b9c1e73c80aab91070c1eb7676f04bdd12d to avoid
>> >> confusion.
>> >
>> >
>> > Why? I believe it fixes a subset of the issue.
>>
>> Could you explain why it fixes a subset of the issue?
>
> From what I remember, I was (before this patch) semi-reliably able to
> reproduce the issue locally, and it went away after. I've observed the same
> thing in the relevant fate station, where before the patch, this warning
> occurred semi-reliably in 3-4 tests per run, and after the patch it
> sometimes occurs in 1 test per run. I understand this doesn't satisfy you
> but I currently don't want to dig through the code to figure out why I
> thought this was a good idea, I have other things that take priority.
> Reverting the patch will require me to dig through this code also, and I
> really don't see the gain.

In addition to avoiding confusion, reverting the patch will move the
av_log() statements outside the lock. Since I don't use those two
av_log() statements, I won't insist on reverting the patch.

> Some more thoughts:
> - I don't think we want to grab a second lock for something trivial like
> this (e.g. grabbing progress_mutex when copying the debug field in
> update_context_from_user);
> - making the debug flags field atomic will be difficult because it's public
> API, and I don't think we're ready to expose C11 types in our public API;
> - I suppose you could make a private (inside PerThreadContext, which allows
> it to be atomic) copy of debug intended for cross-threading purposes and use
> that here.

After studying this problem for two more days, I implemented a variant
of your last suggestion:

http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213384.html

Let's continue the discussion in that email thread.

Wan-Teh Chang