Opened 12 years ago

Closed 11 years ago

#773 closed defect (needs_more_info)

Decoding H.264 gets stuck with Win XP, w32threads, and custom AVCodecContext.get_buffer

Reported by: Andreas Girgensohn Owned by:
Priority: normal Component: avcodec
Version: 0.9 Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

I have an application that decodes H.264 video to in-memory RGB frames. It works fine multi-threaded in Windows 7 or single-threaded in Windows XP. I use FFmpeg 0.9 that includes the patches from Dec. 7 of this functionality in w32threads.h.

FFmpeg is cross-compiled from Fedora 15 with this configuration:

configure --enable-memalign-hack --disable-pthreads --enable-w32threads --arch=x86 --target-os=mingw32 --cross-prefix=i686-pc-mingw32-

I traced the problem to my use of a custom get_buffer, the functions ff_thread_get_buffer and submit_packet, and the new w32threads implementation for Windows XP (not using SleepConditionVariableCS).

After decoding some of the frames, the threads get stuck here:

ff_thread_get_buffer:
while (p->state != STATE_SETTING_UP)

pthread_cond_wait(&p->progress_cond, &p->progress_mutex);

submit_packet:
while (p->state == STATE_SETTING_UP)

pthread_cond_wait(&p->progress_cond, &p->progress_mutex);

As the ffmpeg application does not use a custom get_buffer, it does not have this problem. I tried to figure out what is happening in w32threads.h but unfortunately adding too many print statements makes the problem go away.

I think that the problem can be avoided by using different condition variables for signaling in the two directions between the threads but I haven't tried it.

P.S.: I'm using a custom get_buffer to get a more reliable presentation timestamp. I should check if this is still necessary in the current FFmpeg version.

int64_t global_video_pkt_pts = AV_NOPTS_VALUE;

int getFrameBuffer (AVCodecContext* c, AVFrame* pic) {

int ret = avcodec_default_get_buffer (c, pic);
int64_t* pts = (int64_t*) av_malloc (sizeof (int64_t));
*pts = global_video_pkt_pts;
pic->opaque = pts;
return ret;

}

void releaseFrameBuffer (AVCodecContext* c, AVFrame* pic) {

if (pic)

av_freep (&pic->opaque);

avcodec_default_release_buffer (c, pic);

}

...
global_video_pkt_pts = packet_context.packet.dts;

Change History (6)

comment:1 by reimar, 12 years ago

I don't know who implemented that code and based on what, but it is just broken.
There are gems like this I can't see what purpose they would serve:

pthread_mutex_lock(&win32_cond->mtx_broadcast);
pthread_mutex_unlock(&win32_cond->mtx_broadcast);

However more serious the waiter_count is decremented at the wrong place.
If you look at pthread_cond_wait the WaitForSingleObject is outside of all locks.
This means if somehow all threads leaving that function would be delayed for some time, you could use pthread_cond_signal to arbitrarily increase the value of the semaphore, even though there was never more than one thread waiting.
As a consequence pthread_cond_wait would never be waiting anymore after that and from there it is only downhill.
I can't really (easily at least) explain a hang with that though, there should at least some other things go wrong first.
The method presented here: http://research.microsoft.com/pubs/64242/implementingcvs.pdf looks like it might at least work, even if performance is not exactly good.
Seems their hope of people not repeating mistakes was not exactly justified though.

comment:2 by Andreas Girgensohn, 12 years ago

This Stack Overflow question points to resources for implementing pthread condition variables in Windows XP: http://stackoverflow.com/questions/1218716/implementing-condition-variables-for-critical-sections-for-winthreads-for-xp

Last edited 12 years ago by Andreas Girgensohn (previous) (diff)

comment:3 by Andreas Girgensohn, 12 years ago

Another option would be to use the implementation of pthread_cond_wait and friends from Pthreads-win32: http://sourceware.org/pthreads-win32/

While that project was last updated five years ago, there is a long discussion on the right way to implement condition variables. Also, the source is under LGPL and thus could be used without problems in FFmpeg.

comment:4 by Michael Niedermayer, 12 years ago

This is marked as 0.9, is this also reproduceable with git master ?

comment:5 by Andreas Girgensohn, 12 years ago

Unfortunately, I don't have the implementation with the custom get_buffer anymore. It was intended to get a more reliable presentation timestamp. The current FFmpeg approach for determining timestamps seems to be robust so that it is fine for me.

comment:6 by Carl Eugen Hoyos, 11 years ago

Resolution: needs_more_info
Status: newclosed

Please reopen if this is still reproducible.

Note: See TracTickets for help on using tickets.