*: first attempt to unfuck thread synchronization

I'm already regretting this...

I don't think the author understood how condition variables work,
that signal/broadcast is purely synchronous with blocked waiters
- they don't queue a "signaled" state.

So all these cond_waits littered everywhere without any explicit
stateful condition being watched, if they didn't happen to be
sitting in the wait when the signal occurred, that signal event
was lost, and blocking would persist until the next one.  Even if
the wait only *just* missed the arrival due to scheduling, it was
a very racy and broken use of condition variables.

Not to mention the possibility of spurious wakeups...  You
*always* need some shared state to use condition variables
properly, it's why they include a mutex; to protect the shared
state.  If you're not writing a while() loop to wait on a
condition variable, you're almost certainly doing it wrong.

This stuff will continue to evolve as I get around to it.

7 files changed, 86 insertions, 75 deletions

diff --git a/recordmydesktop/src/rmd_cache_frame.c b/recordmydesktop/src/rmd_cache_frame.c
index 3c4c219..0747f7d 100644
--- a/recordmydesktop/src/rmd_cache_frame.c
+++ b/recordmydesktop/src/rmd_cache_frame.c
@@ -102,7 +102,7 @@ void *rmdCacheImageBuffer(ProgData *pdata) {
 			y_short_blocks[blocknum_x*blocknum_y],
 			u_short_blocks[blocknum_x*blocknum_y],
 			v_short_blocks[blocknum_x*blocknum_y];
-	unsigned long long int total_bytes		  = 0;
+	unsigned long long int total_bytes = 0;
 	unsigned long long int total_received_bytes = 0;
 
 	if (!pdata->args.zerocompression) {
@@ -123,17 +123,16 @@ void *rmdCacheImageBuffer(ProgData *pdata) {
 		FrameHeader fheader;
 		ynum=unum=vnum=0;
 
-		pdata->th_enc_thread_waiting=1;
 		pthread_mutex_lock(&pdata->img_buff_ready_mutex);
-		pthread_cond_wait(&pdata->image_buffer_ready, &pdata->img_buff_ready_mutex);
+		pdata->th_enc_thread_waiting = 1;
+		while (pdata->th_enc_thread_waiting)
+			pthread_cond_wait(&pdata->image_buffer_ready, &pdata->img_buff_ready_mutex);
 		pthread_mutex_unlock(&pdata->img_buff_ready_mutex);
-		pdata->th_enc_thread_waiting=0;
 
-		if (pdata->paused) {
-			pthread_mutex_lock(&pdata->pause_mutex);
+		pthread_mutex_lock(&pdata->pause_mutex);
+		while (pdata->paused)
 			pthread_cond_wait(&pdata->pause_cond, &pdata->pause_mutex);
-			pthread_mutex_unlock(&pdata->pause_mutex);
-		}
+		pthread_mutex_unlock(&pdata->pause_mutex);
 
 		pthread_mutex_lock(&pdata->yuv_mutex);
 
diff --git a/recordmydesktop/src/rmd_encode_image_buffer.c b/recordmydesktop/src/rmd_encode_image_buffer.c
index b96db25..c96412e 100644
--- a/recordmydesktop/src/rmd_encode_image_buffer.c
+++ b/recordmydesktop/src/rmd_encode_image_buffer.c
@@ -33,61 +33,67 @@
 
 
 void *rmdEncodeImageBuffer(ProgData *pdata) {
-	pdata->th_encoding_clean=0;
+	pdata->th_encoding_clean = 0;
+
 	while (pdata->running) {
-		pdata->th_enc_thread_waiting=1;
+		EncData	*enc_data = pdata->enc_data;
+
 		pthread_mutex_lock(&pdata->img_buff_ready_mutex);
-		pthread_cond_wait(&pdata->image_buffer_ready,
-						  &pdata->img_buff_ready_mutex);
+		pdata->th_enc_thread_waiting = 1;
+		while (pdata->th_enc_thread_waiting)
+			pthread_cond_wait(&pdata->image_buffer_ready, &pdata->img_buff_ready_mutex);
+			/* whoever signals us to run sets waiting=0 */
 		pthread_mutex_unlock(&pdata->img_buff_ready_mutex);
-		pdata->th_enc_thread_waiting=0;
-		pdata->encoder_busy = TRUE;
-		if (pdata->paused) {
-			pthread_mutex_lock(&pdata->pause_mutex);
+
+		pthread_mutex_lock(&pdata->pause_mutex);
+		while (pdata->paused)
 			pthread_cond_wait(&pdata->pause_cond, &pdata->pause_mutex);
-			pthread_mutex_unlock(&pdata->pause_mutex);
-		}
+		pthread_mutex_unlock(&pdata->pause_mutex);
+
 		pthread_mutex_lock(&pdata->yuv_mutex);
 
-		if (theora_encode_YUVin(&pdata->enc_data->m_th_st, &pdata->enc_data->yuv)) {
+		if (theora_encode_YUVin(&enc_data->m_th_st, &enc_data->yuv)) {
 			fprintf(stderr,"Encoder not ready!\n");
 			pthread_mutex_unlock(&pdata->yuv_mutex);
 		} else {
 			pthread_mutex_unlock(&pdata->yuv_mutex);
-			if (theora_encode_packetout(&pdata->enc_data->m_th_st, 0, &pdata->enc_data->m_ogg_pckt1)==1) {
+			if (theora_encode_packetout(&enc_data->m_th_st, 0, &enc_data->m_ogg_pckt1) == 1) {
+
 				pthread_mutex_lock(&pdata->libogg_mutex);
-				ogg_stream_packetin(&pdata->enc_data->m_ogg_ts,
-									&pdata->enc_data->m_ogg_pckt1);
+				ogg_stream_packetin(&enc_data->m_ogg_ts, &enc_data->m_ogg_pckt1);
+				pdata->avd += pdata->frametime;
 				pthread_mutex_unlock(&pdata->libogg_mutex);
-				pdata->avd+=pdata->frametime;
 			}
 		}
-		pdata->encoder_busy = FALSE;
 	}
+
 	//last packet
-	pdata->th_encoding_clean=1;
 	pthread_mutex_lock(&pdata->theora_lib_mutex);
+	pdata->th_encoding_clean = 1;
 	pthread_cond_signal(&pdata->theora_lib_clean);
 	pthread_mutex_unlock(&pdata->theora_lib_mutex);
+
 	pthread_exit(&errno);
 }
 
 //this function is meant to be called normally
 //not through a thread of it's own
 void rmdSyncEncodeImageBuffer(ProgData *pdata) {
-	if (theora_encode_YUVin(&pdata->enc_data->m_th_st, &pdata->enc_data->yuv)) {
+	EncData	*enc_data = pdata->enc_data;
+
+	if (theora_encode_YUVin(&enc_data->m_th_st, &enc_data->yuv)) {
 		fprintf(stderr,"Encoder not ready!\n");
 		return;
 	}
 
-	if (theora_encode_packetout(&pdata->enc_data->m_th_st, !pdata->running, &pdata->enc_data->m_ogg_pckt1)==1) {
+	if (theora_encode_packetout(&enc_data->m_th_st, !pdata->running, &enc_data->m_ogg_pckt1) == 1) {
 		pthread_mutex_lock(&pdata->libogg_mutex);
-		ogg_stream_packetin(&pdata->enc_data->m_ogg_ts, &pdata->enc_data->m_ogg_pckt1);
+		ogg_stream_packetin(&enc_data->m_ogg_ts, &enc_data->m_ogg_pckt1);
 
 		if (!pdata->running)
-			pdata->enc_data->m_ogg_ts.e_o_s=1;
+			enc_data->m_ogg_ts.e_o_s=1;
 
+		pdata->avd += pdata->frametime;
 		pthread_mutex_unlock(&pdata->libogg_mutex);
-		pdata->avd+=pdata->frametime;
 	}
 }
diff --git a/recordmydesktop/src/rmd_get_frame.c b/recordmydesktop/src/rmd_get_frame.c
index 229cb61..e153d15 100644
--- a/recordmydesktop/src/rmd_get_frame.c
+++ b/recordmydesktop/src/rmd_get_frame.c
@@ -422,28 +422,25 @@ void *rmdGetFrame(ProgData *pdata) {
 		//if we are left behind we must not wait.
 		//also before actually pausing we must make sure the streams
 		//are synced. sound stops so this should only happen quickly.
-		if (pdata->avd>0 || pdata->args.nosound) {
-			pthread_mutex_lock(&pdata->time_mutex);
-			pthread_cond_wait(&pdata->time_cond, &pdata->time_mutex);
-			pthread_mutex_unlock(&pdata->time_mutex);
-
-			if (pdata->paused) {
-				//this is necessary since event loop processes
-				//the shortcuts which will unpause the program
-				rmdEventLoop(pdata);
-				continue;
-			}
-		}
+		pthread_mutex_lock(&pdata->time_mutex);
+		while (	(pdata->avd > 0 || pdata->args.nosound) &&
+			pdata->capture_frameno >= pdata->time_frameno)
+				pthread_cond_wait(&pdata->time_cond, &pdata->time_mutex);
+
+		pdata->capture_frameno = pdata->time_frameno;
+		pthread_mutex_unlock(&pdata->time_mutex);
+
 		//read all events and construct list with damage 
 		//events (if not full_shots)
 		rmdEventLoop(pdata);
 
+		if (pdata->paused)
+			continue;
+
 		//switch back and front buffers (full_shots only)
 		if (d_buff)
 			img_sel=(img_sel)?0:1;
 
-		pdata->capture_busy = TRUE;
-
 		rmdBRWinCpy(&temp_brwin, &pdata->brwin);
 
 
@@ -664,21 +661,21 @@ void *rmdGetFrame(ProgData *pdata) {
 		if (!pdata->args.full_shots)
 			rmdClearList(&pdata->rect_root);
 
-		if (pdata->encoder_busy)
-			pdata->frames_lost++;
 
 		pthread_mutex_lock(&pdata->img_buff_ready_mutex);
-		pthread_cond_broadcast(&pdata->image_buffer_ready);
+		pdata->th_enc_thread_waiting = 0;
+		pthread_cond_signal(&pdata->image_buffer_ready);
 		pthread_mutex_unlock(&pdata->img_buff_ready_mutex);
-		pdata->capture_busy = FALSE;
 	}
 
 	if (!pdata->args.noframe)
 		XDestroyWindow(pdata->dpy,pdata->shaped_w);
 
 	pthread_mutex_lock(&pdata->img_buff_ready_mutex);
-	pthread_cond_broadcast(&pdata->image_buffer_ready);
+	pdata->th_enc_thread_waiting = 0;
+	pthread_cond_signal(&pdata->image_buffer_ready);
 	pthread_mutex_unlock(&pdata->img_buff_ready_mutex);
+
 	if (!pdata->args.noshared) {
 		XShmDetach (pdata->dpy, &shminfo);
 		shmdt (shminfo.shmaddr);
@@ -689,5 +686,6 @@ void *rmdGetFrame(ProgData *pdata) {
 			shmctl (shminfo_back.shmid, IPC_RMID, 0);
 		}
 	}
+
 	pthread_exit(&errno);
 }
diff --git a/recordmydesktop/src/rmd_initialize_data.c b/recordmydesktop/src/rmd_initialize_data.c
index 69f4fce..ecf708a 100644
--- a/recordmydesktop/src/rmd_initialize_data.c
+++ b/recordmydesktop/src/rmd_initialize_data.c
@@ -99,8 +99,8 @@ int rmdInitializeData(ProgData *pdata, EncData *enc_data, CacheData *cache_data)
 	pdata->pause_state_changed	= FALSE;
 	pdata->frames_total		= 0;
 	pdata->frames_lost		= 0;
-	pdata->encoder_busy		= FALSE;
-	pdata->capture_busy		= FALSE;
+	pdata->capture_frameno		= 0;
+	pdata->time_frameno		= 0;
 
 	if (!pdata->args.nosound) {
 		if (!pdata->args.use_jack) {
diff --git a/recordmydesktop/src/rmd_threads.c b/recordmydesktop/src/rmd_threads.c
index 7afe792..91823b4 100644
--- a/recordmydesktop/src/rmd_threads.c
+++ b/recordmydesktop/src/rmd_threads.c
@@ -119,11 +119,13 @@ void rmdThreads(ProgData *pdata) {
 	pthread_join(image_capture_t,NULL);
 	fprintf(stderr,"Shutting down.");
 	//if no damage events have been received the thread will get stuck
-	while (!pdata->th_enc_thread_waiting && !pdata->th_encoding_clean) {
-		usleep(10000);
-		pthread_mutex_lock(&pdata->img_buff_ready_mutex);
+	pthread_mutex_lock(&pdata->img_buff_ready_mutex);
+	while (pdata->th_enc_thread_waiting && !pdata->th_encoding_clean) {
+		puts("waiting for th_enc");
+		pdata->th_enc_thread_waiting = 0;
 		pthread_cond_signal(&pdata->image_buffer_ready);
 		pthread_mutex_unlock(&pdata->img_buff_ready_mutex);
+		usleep(10000);
 	}
 
 	if (pdata->args.encOnTheFly)
@@ -141,11 +143,13 @@ void rmdThreads(ProgData *pdata) {
 			pthread_join(sound_capture_t,NULL);
 
 		fprintf(stderr,".");
+		pthread_mutex_lock(&pdata->snd_buff_ready_mutex);
 		while (pdata->v_enc_thread_waiting || !pdata->v_encoding_clean) {
-			usleep(10000);
-			pthread_mutex_lock(&pdata->snd_buff_ready_mutex);
+			puts("waiting for v_enc");
+			pdata->v_enc_thread_waiting = 0;
 			pthread_cond_signal(&pdata->sound_data_read);
 			pthread_mutex_unlock(&pdata->snd_buff_ready_mutex);
+			usleep(10000);
 		}
 
 		if (pdata->args.encOnTheFly)
diff --git a/recordmydesktop/src/rmd_timer.c b/recordmydesktop/src/rmd_timer.c
index 2548fbe..b8161ca 100644
--- a/recordmydesktop/src/rmd_timer.c
+++ b/recordmydesktop/src/rmd_timer.c
@@ -41,7 +41,9 @@ void *rmdTimer(ProgData *pdata){
 	long unsigned int secs_tw=1/pdata->args.fps;
 	long unsigned int usecs_tw=(1000000)/pdata->args.fps- secs_tw*1000000;
 
-	while (pdata->timer_alive){
+	while (pdata->timer_alive) {
+
+		pthread_mutex_lock(&pdata->pause_mutex);
 		if (pdata->pause_state_changed) {
 			pdata->pause_state_changed = FALSE;
 
@@ -51,22 +53,24 @@ void *rmdTimer(ProgData *pdata){
 			} else{
 				pdata->paused = FALSE;
 				fprintf(stderr,"STATE:RECORDING\n");fflush(stderr);
-				pthread_mutex_lock(&pdata->pause_mutex);
 				pthread_cond_broadcast(&pdata->pause_cond);
-				pthread_mutex_unlock(&pdata->pause_mutex);
 			}
 		}
 
 		if (!pdata->paused) {
+			pthread_mutex_unlock(&pdata->pause_mutex);
+
+			/* FIXME TODO: detect dropped frames by delta between {time,capture}_frameno */
 			pdata->frames_total++;
-			if (pdata->capture_busy)
-				pdata->frames_lost++;
-		}
-		
+		} else
+			pthread_mutex_unlock(&pdata->pause_mutex);
+
 		pthread_mutex_lock(&pdata->time_mutex);
-		pthread_cond_broadcast(&pdata->time_cond);
+		pdata->time_frameno++;
 		pthread_mutex_unlock(&pdata->time_mutex);
+		pthread_cond_signal(&pdata->time_cond);
 
+		/* FIXME use nanosleep */
 		if (secs_tw)
 			sleep(secs_tw);
 
diff --git a/recordmydesktop/src/rmd_types.h b/recordmydesktop/src/rmd_types.h
index 218d832..cd2cca4 100644
--- a/recordmydesktop/src/rmd_types.h
+++ b/recordmydesktop/src/rmd_types.h
@@ -277,16 +277,16 @@ struct _ProgData {
 								//Currently this mutex only prevents
 								//the cursor from flickering
 /**Condition Variables*/
-	pthread_cond_t  time_cond,  //this gets a broadcast by the handler
+	pthread_cond_t			time_cond,		//this gets a broadcast by the handler
 								//whenever it's time to get a screenshot
-					pause_cond, //this is blocks execution,
+					pause_cond,		//this is blocks execution,
 								//when program is paused
 					sound_data_read,	//a buffer is ready for proccessing
-					image_buffer_ready, //image encoding finished
-					theora_lib_clean,   //the flush_ogg thread cannot
-										//procceed to creating last
-					vorbis_lib_clean;   //packages until these two libs
-										//are no longer used, by other threads
+					image_buffer_ready,	//image encoding finished
+					theora_lib_clean,	//the flush_ogg thread cannot
+								//procceed to creating last
+					vorbis_lib_clean;	//packages until these two libs
+								//are no longer used, by other threads
 /**Buffers,Flags and other vars*/
 	unsigned char *dummy_pointer,   //a dummy pointer to be drawn
 									//in every frame
@@ -322,10 +322,10 @@ struct _ProgData {
 	unsigned int	frames_total,   //frames calculated by total time expirations
 			frames_lost;	//the value of shame
 
-	//used to determine frame drop which can
-	//happen on failure to receive a signal over a condition variable
-	boolean capture_busy,
-			encoder_busy;
+	/* timer advances time_frameno, getframe copies time_frameno to capture_frameno
+	 * access to both is serialized by time_{mutex,cond}
+	 */
+	unsigned int	time_frameno, capture_frameno;
 
 	pthread_mutex_t pause_mutex;
 	pthread_mutex_t time_mutex;