Close Thread Handle in avcodec_thread_free
Hi guy's I am willing to post a patch for avcodec_thread_free. When using multi threaded video decoding and upon closure in avcodec_thread_free thread handle are not being closed But I have no idea to how to create a diff file or patch file on windows OS. Only one line is added in avcodec_thread_free function which is CloseHandle(c[i].thread); void avcodec_thread_free(AVCodecContext *s){ ThreadContext *c= s->thread_opaque; int i; for(i=0; i<s->thread_count; i++){ c[i].func= NULL; ReleaseSemaphore(c[i].work_sem, 1, 0); WaitForSingleObject(c[i].thread, INFINITE); if(c[i].work_sem) CloseHandle(c[i].work_sem); if(c[i].done_sem) CloseHandle(c[i].done_sem); CloseHandle(c[i].thread); } av_freep(&s->thread_opaque); } Thanks -- ?????? ????? ????? ???? ?????? Best Regards Shehzad Salim
On Fri, Aug 07, 2009 at 12:59:07PM +0100, Shehzad Salim wrote:
But I have no idea to how to create a diff file or patch file on windows OS.
Have you ever tried TortoiseSVN? Not only can you easily get the latest SVN of FFmpeg (which patches have to be against), you also can also easily create diffs of the changes you made. Also several IDEs (like eclipse) have at least optional plugins for SVN support that can do that kind of thing, too. Of course you can just use the commandline subversion client in exactly the same way you do on Linux or so.
Only one line is added in avcodec_thread_free function which is CloseHandle(c[i].thread);
void avcodec_thread_free(AVCodecContext *s){ ThreadContext *c= s->thread_opaque; int i;
for(i=0; i<s->thread_count; i++){
c[i].func= NULL; ReleaseSemaphore(c[i].work_sem, 1, 0); WaitForSingleObject(c[i].thread, INFINITE); if(c[i].work_sem) CloseHandle(c[i].work_sem); if(c[i].done_sem) CloseHandle(c[i].done_sem); CloseHandle(c[i].thread); }
av_freep(&s->thread_opaque); }
Ok, I read it up. This is indeed correct since we use _beginthreadex. It is also incomplete though, the thread function must call _endthreadex, the return 0 is wrong and may leak memory, too (well, actually the documentation contradicts itself here, it says that _endthreadex will be called automatically anyway, but also says that calling it will help reclaiming resources, the exact quote: "You can call _endthread or _endthreadex explicitly to terminate a thread; however, _endthread or _endthreadex is called automatically when the thread returns from the routine passed as a parameter. Terminating a thread with a call to endthread or _endthreadex helps to ensure proper recovery of resources allocated for the thread.").
On Fri, Aug 7, 2009 at 3:41 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de>wrote:
On Fri, Aug 07, 2009 at 12:59:07PM +0100, Shehzad Salim wrote:
But I have no idea to how to create a diff file or patch file on windows OS.
Have you ever tried TortoiseSVN? Not only can you easily get the latest SVN of FFmpeg (which patches have to be against), you also can also easily create diffs of the changes you made. Also several IDEs (like eclipse) have at least optional plugins for SVN support that can do that kind of thing, too. Of course you can just use the commandline subversion client in exactly the same way you do on Linux or so.
Thanks a lot for telling about TortoiseSVN
Only one line is added in avcodec_thread_free function which is CloseHandle(c[i].thread);
void avcodec_thread_free(AVCodecContext *s){ ThreadContext *c= s->thread_opaque; int i;
for(i=0; i<s->thread_count; i++){
c[i].func= NULL; ReleaseSemaphore(c[i].work_sem, 1, 0); WaitForSingleObject(c[i].thread, INFINITE); if(c[i].work_sem) CloseHandle(c[i].work_sem); if(c[i].done_sem) CloseHandle(c[i].done_sem); CloseHandle(c[i].thread); }
av_freep(&s->thread_opaque); }
Ok, I read it up. This is indeed correct since we use _beginthreadex. It is also incomplete though, the thread function must call _endthreadex, the return 0 is wrong and may leak memory, too (well, actually the documentation contradicts itself here, it says that _endthreadex will be called automatically anyway, but also says that calling it will help reclaiming resources, the exact quote: "You can call _endthread or _endthreadex explicitly to terminate a thread; however, _endthread or _endthreadex is called automatically when the thread returns from the routine passed as a parameter. Terminating a thread with a call to endthread or _endthreadex helps to ensure proper recovery of resources allocated for the thread.").
I guess call to WaitForSingleObject is making sure that thread is finished, automatically either by _endthread or _endthreadex. But it's handle never get's closed. Call to CloseHandle makes sure that it is closed and now I don't have any more problems. Windows OS behaves very badly once process handle count goes above 7 or 8 thousand. Please find enclosed patch for w32thread.c
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- ?????? ????? ????? ???? ?????? Best Regards Shehzad Salim
On Fri, Aug 7, 2009 at 4:31 PM, Shehzad Salim <shehzadsalim at gmail.com>wrote:
On Fri, Aug 7, 2009 at 3:41 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de
wrote:
On Fri, Aug 07, 2009 at 12:59:07PM +0100, Shehzad Salim wrote:
But I have no idea to how to create a diff file or patch file on windows OS.
Have you ever tried TortoiseSVN? Not only can you easily get the latest SVN of FFmpeg (which patches have to be against), you also can also easily create diffs of the changes you made. Also several IDEs (like eclipse) have at least optional plugins for SVN support that can do that kind of thing, too. Of course you can just use the commandline subversion client in exactly the same way you do on Linux or so.
Thanks a lot for telling about TortoiseSVN
Only one line is added in avcodec_thread_free function which is CloseHandle(c[i].thread);
void avcodec_thread_free(AVCodecContext *s){ ThreadContext *c= s->thread_opaque; int i;
for(i=0; i<s->thread_count; i++){
c[i].func= NULL; ReleaseSemaphore(c[i].work_sem, 1, 0); WaitForSingleObject(c[i].thread, INFINITE); if(c[i].work_sem) CloseHandle(c[i].work_sem); if(c[i].done_sem) CloseHandle(c[i].done_sem); CloseHandle(c[i].thread); }
av_freep(&s->thread_opaque); }
Ok, I read it up. This is indeed correct since we use _beginthreadex. It is also incomplete though, the thread function must call _endthreadex, the return 0 is wrong and may leak memory, too (well, actually the documentation contradicts itself here, it says that _endthreadex will be called automatically anyway, but also says that calling it will help reclaiming resources, the exact quote: "You can call _endthread or _endthreadex explicitly to terminate a thread; however, _endthread or _endthreadex is called automatically when the thread returns from the routine passed as a parameter. Terminating a thread with a call to endthread or _endthreadex helps to ensure proper recovery of resources allocated for the thread.").
I guess call to WaitForSingleObject is making sure that thread is finished, automatically either by _endthread or _endthreadex. But it's handle never get's closed. Call to CloseHandle makes sure that it is closed and now I don't have any more problems. Windows OS behaves very badly once process handle count goes above 7 or 8 thousand.
Please find enclosed patch for w32thread.c
Is there anything else I need to do regarding this patch, so that it is committed. This patch surely does fix Thread Handle leak issue. It is not very big patch as well. Thanks
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- ?????? ????? ????? ???? ??????
Best Regards Shehzad Salim
-- ?????? ????? ????? ???? ?????? Best Regards Shehzad Salim
On Wed, Aug 12, 2009 at 01:31:40PM +0100, Shehzad Salim wrote:
On Fri, Aug 7, 2009 at 4:31 PM, Shehzad Salim <shehzadsalim at gmail.com>wrote:
But it's handle never get's closed. Call to CloseHandle makes sure that it is closed and now I don't have any more problems. Windows OS behaves very badly once process handle count goes above 7 or 8 thousand.
Please find enclosed patch for w32thread.c
Is there anything else I need to do regarding this patch, so that it is committed. This patch surely does fix Thread Handle leak issue. It is not very big patch as well.
I was hoping someone who actually is using Windows would speak up or something. Well, I just applied it now, tell me if there are any issues with it (I admit I did not actually test it).
On Wed, Aug 12, 2009 at 2:27 PM, Reimar D?ffinger <Reimar.Doeffinger at gmx.de>wrote:
On Wed, Aug 12, 2009 at 01:31:40PM +0100, Shehzad Salim wrote:
On Fri, Aug 7, 2009 at 4:31 PM, Shehzad Salim <shehzadsalim at gmail.com wrote:
But it's handle never get's closed. Call to CloseHandle makes sure that it is closed and now I don't have any more problems. Windows OS behaves very badly once process handle count goes above 7 or 8 thousand.
Please find enclosed patch for w32thread.c
Is there anything else I need to do regarding this patch, so that it is committed. This patch surely does fix Thread Handle leak issue. It is not very big patch as well.
I was hoping someone who actually is using Windows would speak up or something. Well, I just applied it now, tell me if there are any issues with it (I admit I did not actually test it).
Thanks a lot. It is included in main stream. It has been tested over 2 nights without any resets Once again thanks :)
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- ?????? ????? ????? ???? ?????? Best Regards Shehzad Salim
Hi,
Propagating the error code back to the caller would be nice. How?
And should I also fix r210 decoder because the code are copy-pasted from r210dec.c. ZZ
Hi, New patch here. ZZ
2010/6/27 Zhou Zongyi <zhouzy at os.pku.edu.cn>:
Hi,
New patch here.
Hello, I thought I'd provide you with a suggestion to get this committed so you can cleanup your patch(es) for your bitstream filter. I've attached a patch for the decoder which removes most of the duplication. Almost everything is identical to the r210 decoder. One question, why do you use: *dst++ = r + (r >> 10); *dst++ = g + (g >> 10); *dst++ = b + (b >> 10); When the original does: *dst++ = r | (r >> 10); *dst++ = g | (g >> 10); *dst++ = b | (b >> 10);
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote: [...]
@@ -102,3 +110,18 @@ AVCodec r210_decoder = { CODEC_CAP_DR1, .long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"), [...] + .long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"),
long name should be unique [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
On 16 August 2010 15:34, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote: [...]
@@ -102,3 +110,18 @@ AVCodec r210_decoder = { ? ? ?CODEC_CAP_DR1, ? ? ?.long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"), [...] + ? ?.long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"),
long name should be unique
How about "AJA Kona 10-bit RGB Codec" for r10k? That's in the sample file from mplayer and listed on the multimedia wiki
On Tue, 17 Aug 2010 14:26:51 +0200, James Darnley wrote:
On 16 August 2010 15:34, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote: [...]
@@ -102,3 +110,18 @@ AVCodec r210_decoder = { ? ? ?CODEC_CAP_DR1, ? ? ?.long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"), [...] + ? ?.long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"),
long name should be unique
How about "AJA Kona 10-bit RGB Codec" for r10k? That's in the sample file from mplayer and listed on the multimedia wiki
hard to find information on it on AJA's site. "The 'R10k' pixel format is defined by AJA." http://www.bitjazz.com/en/products/sheervideo/faq/formats/pixel_formats.php#... -compn
we have r10g samples too. are they similar to r10k? http://samples.mplayerhq.hu/V-codecs/ http://samples.mplayerhq.hu/V-codecs/R10g.mov http://samples.mplayerhq.hu/V-codecs/R10g-LE.mov -compn
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote:
I've attached a patch for the decoder which removes most of the duplication. Almost everything is identical to the r210 decoder.
This is missing a changelog and documentation update. Diego
On 23 August 2010 15:40, Diego Biurrun <diego at biurrun.de> wrote:
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote:
I've attached a patch for the decoder which removes most of the duplication. ?Almost everything is identical to the r210 decoder.
This is missing a changelog and documentation update.
I've done the changelog. By the docs you mean doc/general.texi which lists the available encoders/decoders? If so, then I've done that. I duplicated the r210 line below and replaced the parts as needed. If/when you commit, don't forget to credit Zhou Zongyi for the original work. Changes from the previous patch are below.
diff --git a/Changelog b/Changelog index 70849ea..29b4d8e 100644 --- a/Changelog +++ b/Changelog @@ -34,6 +34,7 @@ version <next>: - a64 codec - MMS-HTTP support - G.722 ADPCM audio decoder +- R10k video decoder
version 0.6: diff --git a/doc/general.texi b/doc/general.texi index a692e04..9a3200f 100644 --- a/doc/general.texi +++ b/doc/general.texi @@ -460,6 +460,7 @@ following image formats are supported: @tab fourcc: 'smc ' @item QuickTime video (RPZA) @tab @tab X @tab fourcc: rpza + at item R10K AJA Kona 10-bit RGB Codec @tab @tab X @item R210 Quicktime Uncompressed RGB 10-bit @tab @tab X @item Raw Video @tab X @tab X @item RealVideo 1.0 @tab X @tab X diff --git a/libavcodec/r210dec.c b/libavcodec/r210dec.c index 5916feb..b1fe62d 100644 --- a/libavcodec/r210dec.c +++ b/libavcodec/r210dec.c @@ -121,6 +121,6 @@ AVCodec r10k_decoder = { decode_close, decode_frame, CODEC_CAP_DR1, - .long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"), + .long_name = NULL_IF_CONFIG_SMALL("AJA Kona 10-bit RGB Codec"), }; #endif
James Darnley <james.darnley <at> gmail.com> writes:
If/when you commit, don't forget to credit Zhou Zongyi for the original work.
Changes from the previous patch are below.
Sorry that this wasn't mentioned before, but please do not re-indent the three unchanged lines in r210dec.c, it makes reading the patch harder. Carl Eugen
On Mon, Sep 13, 2010 at 11:31:11PM +0200, James Darnley wrote:
On 23 August 2010 15:40, Diego Biurrun <diego at biurrun.de> wrote:
On Sun, Aug 15, 2010 at 05:40:52PM +0200, James Darnley wrote:
I've attached a patch for the decoder which removes most of the duplication. ?Almost everything is identical to the r210 decoder.
This is missing a changelog and documentation update.
I've done the changelog. By the docs you mean doc/general.texi which lists the available encoders/decoders? If so, then I've done that. I duplicated the r210 line below and replaced the parts as needed.
If/when you commit, don't forget to credit Zhou Zongyi for the original work.
Changes from the previous patch are below.
diff --git a/Changelog b/Changelog index 70849ea..29b4d8e 100644 --- a/Changelog +++ b/Changelog @@ -34,6 +34,7 @@ version <next>: - a64 codec - MMS-HTTP support - G.722 ADPCM audio decoder +- R10k video decoder
version 0.6: diff --git a/doc/general.texi b/doc/general.texi index a692e04..9a3200f 100644 --- a/doc/general.texi +++ b/doc/general.texi @@ -460,6 +460,7 @@ following image formats are supported: @tab fourcc: 'smc ' @item QuickTime video (RPZA) @tab @tab X @tab fourcc: rpza + at item R10K AJA Kona 10-bit RGB Codec @tab @tab X @item R210 Quicktime Uncompressed RGB 10-bit @tab @tab X @item Raw Video @tab X @tab X @item RealVideo 1.0 @tab X @tab X diff --git a/libavcodec/r210dec.c b/libavcodec/r210dec.c index 5916feb..b1fe62d 100644 --- a/libavcodec/r210dec.c +++ b/libavcodec/r210dec.c @@ -121,6 +121,6 @@ AVCodec r10k_decoder = { decode_close, decode_frame, CODEC_CAP_DR1, - .long_name = NULL_IF_CONFIG_SMALL("Uncompressed RGB 10-bit"), + .long_name = NULL_IF_CONFIG_SMALL("AJA Kona 10-bit RGB Codec"), }; #endif
patches should be ok if tested [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
Michael Niedermayer <michaelni <at> gmx.at> writes:
patches should be ok if tested
Patch applied. Carl Eugen
participants (8)
-
cehoyos＠ag.or.at -
diego＠biurrun.de -
james.darnley＠gmail.com -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de -
shehzadsalim＠gmail.com -
tempn＠twmi.rr.com -
zhouzy＠os.pku.edu.cn