Hi, On Tue, Mar 1, 2016 at 1:47 PM, Wan-Teh Chang <wtc-at-google....@ffmpeg.org> wrote:
> Hi Ronald, > > On Fri, Feb 26, 2016 at 12:16 PM, Ronald S. Bultje <rsbul...@gmail.com> > wrote: > > > > This will be slower, right? Is this an actual issue? Like, can you point > to > > cases like [1] (i.e. a real-world race condition with real-world > > consequences) that were fixed with your patch? > > > > Ronald > > > > https://blogs.gnome.org/rbultje/2014/01/12/brute-force-thread-debugging/ > > With the new relaxed and acquire/release atomic int get and set > functions I proposed in > http://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/190516.html, we > have the means to make the current code portable while preserving the > exact performance for x86/x86_64. But unlike the progress[field] > values, I am strongly against using this approach for |state| for two > reasons. > > 1) |state| is guarded by one of two mutexes: |mutex| and > |progress_mutex|. Which mutex should guard |state| when is not > documented, and I haven't reverse-engineered that policy. I can help there. state is an enum with the following values: STATE_INPUT_READY STATE_SETTING_UP STATE_GET_BUFFER STATE_GET_FORMAT STATE_SETUP_FINISHED Transitions are like this: - INPUT_READY to SETTING_UP in submit_packet() (main thread), guarded by mutex. frame_worker_thread() (frame decoder thread[s]) awaits this state change holding mutex. The accompanying cond is input_cond. - any transition between SETTING_UP, GET_BUFFER and GET_FORMAT in submit_packet (main thread) and thread_get_buffer_internal/ff_thread_get_format (decoder thread[s]), guarded by progress_mutex. The accompanying cond is progress_cond. - SETTING_UP/GET_BUFFER/GET_FORMAT to SETUP_FINISHED in ff_thread_finish_setup() (decoder thread[s]), guarded by progress_mutex. The accompanying cond is progress_cond. This condition is awaited before submit_packet() (main thread) returns to ensure that get_formats and get_buffer was called in the context of the call to submit_packet(), assuming the application did not explicitly say that calls to these callbacks are threadsafe. I agree that the code at the bottom of submit_packet() is ... Strange :) But I'm not convinced it's buggy. I'm sure tsan hates it. I wouldn't mind patches that clean it up assuming they don't make the code harder to read or slower. - SETUP_FINISHED to INPUT_READY is in frame_worker_thread() (decoder thread[s]) and is guarded by both mutex as well as progress_mutex. This signals output_cond and progress_cond (both under control of progress_mutex). ff_thread_decode_frame() (main thread) uses this transition controlled by output_cond to return decoded frames to the application after calling submit_packet(). Hope this helps, Ronald _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
