<div dir="ltr"><div>Any reason why threadLocalData->nr needs to use malloc and not new?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 15, 2014 at 5:54 AM, Deepthi Nandakumar <span dir="ltr"><<a href="mailto:deepthi@multicorewareinc.com" target="_blank">deepthi@multicorewareinc.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Aah, you' re right. In my older patch, the encoder was initialising threadLocalData.nr . Somehow, I missed that while making the new patch.<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 15, 2014 at 12:41 AM, Steve Borho <span dir="ltr"><<a href="mailto:steve@borho.org" target="_blank">steve@borho.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On 10/14, <a href="mailto:deepthi@multicorewareinc.com" target="_blank">deepthi@multicorewareinc.com</a> wrote:<br>
> # HG changeset patch<br>
> # User Deepthi Nandakumar <<a href="mailto:deepthi@multicorewareinc.com" target="_blank">deepthi@multicorewareinc.com</a>><br>
> # Date 1413278604 -19800<br>
> # Tue Oct 14 14:53:24 2014 +0530<br>
> # Node ID c6e786dbbfaa39822799d17e6c32d49c6141a7fb<br>
> # Parent 38b5733cc629dd16db770e6a93b4f994e13336f3<br>
> noiseReduction: make noiseReduction deterministic for a given number of frameEncoders.<br>
><br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/common/frame.cpp<br>
> --- a/source/common/frame.cpp Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/common/frame.cpp Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -43,6 +43,7 @@<br>
> m_picSym = NULL;<br>
> m_reconRowCount.set(0);<br>
> m_countRefEncoders = 0;<br>
> + m_frameEncoderID = 0;<br>
> memset(&m_lowres, 0, sizeof(m_lowres));<br>
> m_next = NULL;<br>
> m_prev = NULL;<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/common/frame.h<br>
> --- a/source/common/frame.h Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/common/frame.h Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -50,6 +50,7 @@<br>
> TComPicSym* m_picSym;<br>
> TComPicYuv* m_reconPicYuv;<br>
> int m_POC;<br>
> + int m_frameEncoderID; // To identify the ID of the frameEncoder processing this frame<br>
><br>
> //** Frame Parallelism - notification between FrameEncoders of available motion reference rows **<br>
> ThreadSafeInteger m_reconRowCount; // count of CTU rows completely reconstructed and extended for motion reference<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/common/quant.cpp<br>
> --- a/source/common/quant.cpp Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/common/quant.cpp Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -156,6 +156,7 @@<br>
> m_resiDctCoeff = NULL;<br>
> m_fencDctCoeff = NULL;<br>
> m_fencShortBuf = NULL;<br>
> + m_nr = NULL;<br>
> }<br>
><br>
> bool Quant::init(bool useRDOQ, double psyScale, const ScalingList& scalingList, Entropy& entropy)<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/analysis.cpp<br>
> --- a/source/encoder/analysis.cpp Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/analysis.cpp Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -292,7 +292,11 @@<br>
> if (!jobId || m_param->rdLevel > 4)<br>
> {<br>
> slave->m_quant.setQPforQuant(cu);<br>
> - slave->m_quant.m_nr = m_quant.m_nr;<br>
> + if(m_param->noiseReduction)<br>
<br>
</div></div>w-s<br>
<span><br>
> + {<br>
> + int frameEncoderID = cu->m_pic->m_frameEncoderID;<br>
> + slave->m_quant.m_nr = &m_tld[threadId].m_nr[frameEncoderID];<br>
> + }<br>
> slave->m_rdContexts[depth].cur.load(m_rdContexts[depth].cur);<br>
> }<br>
> }<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/analysis.h<br>
> --- a/source/encoder/analysis.h Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/analysis.h Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -172,7 +172,9 @@<br>
> struct ThreadLocalData<br>
> {<br>
> Analysis analysis;<br>
> -<br>
> + NoiseReduction *m_nr;<br>
> +<br>
> + ThreadLocalData() { m_nr = NULL; }<br>
> ~ThreadLocalData() { analysis.destroy(); }<br>
<br>
</span>ThreadLocalData is a struct, so members don't get m_ prefixes.<br>
<br>
The destructor should be removed and make a destroy() function which<br>
releases the noise reduction array and calls analysis.destroy();<br>
<div><div><br>
> };<br>
><br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/encoder.cpp<br>
> --- a/source/encoder/encoder.cpp Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/encoder.cpp Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -74,6 +74,7 @@<br>
> m_csvfpt = NULL;<br>
> m_param = NULL;<br>
> m_threadPool = 0;<br>
> + m_numThreadLocalData = 0;<br>
> }<br>
><br>
> void Encoder::create()<br>
> @@ -162,15 +163,17 @@<br>
><br>
> /* Allocate thread local data, one for each thread pool worker and<br>
> * if --no-wpp, one for each frame encoder */<br>
> - int numLocalData = poolThreadCount;<br>
> + m_numThreadLocalData = poolThreadCount;<br>
> if (!m_param->bEnableWavefront)<br>
> - numLocalData += m_param->frameNumThreads;<br>
> - m_threadLocalData = new ThreadLocalData[numLocalData];<br>
> - for (int i = 0; i < numLocalData; i++)<br>
> + m_numThreadLocalData += m_param->frameNumThreads;<br>
> + m_threadLocalData = new ThreadLocalData[m_numThreadLocalData];<br>
> + for (int i = 0; i < m_numThreadLocalData; i++)<br>
> {<br>
> m_threadLocalData[i].analysis.setThreadPool(m_threadPool);<br>
> m_threadLocalData[i].analysis.initSearch(m_param, m_scalingList);<br>
> m_threadLocalData[i].analysis.create(g_maxCUDepth + 1, g_maxCUSize, m_threadLocalData);<br>
> + if(m_param->noiseReduction)<br>
> + m_threadLocalData[i].m_nr = new NoiseReduction[m_param->frameNumThreads];<br>
<br>
</div></div>w-s, and this should be X265_MALLOC(NoiseReduction, m_param->frameNumThreads);<br>
<span><br>
> }<br>
><br>
> if (!m_param->bEnableWavefront)<br>
> @@ -240,6 +243,12 @@<br>
> delete [] m_frameEncoder;<br>
> }<br>
><br>
> + if (m_param->noiseReduction)<br>
> + {<br>
> + for (int i = 0; i < m_numThreadLocalData; i++)<br>
> + delete [] m_threadLocalData[i].m_nr;<br>
<br>
</span>use destroy() for this<br>
<span><br>
> + }<br>
> +<br>
> delete [] m_threadLocalData;<br>
><br>
> if (m_lookahead)<br>
> @@ -400,6 +409,7 @@<br>
> m_lookahead->flush();<br>
><br>
> FrameEncoder *curEncoder = &m_frameEncoder[m_curEncoder];<br>
> + curEncoder->m_frameEncoderID = m_curEncoder;<br>
<br>
</span>this is too opaque. better to pass the ID to the frame encoder init()<br>
function<br>
<div><div><br>
> m_curEncoder = (m_curEncoder + 1) % m_param->frameNumThreads;<br>
> int ret = 0;<br>
><br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/encoder.h<br>
> --- a/source/encoder/encoder.h Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/encoder.h Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -116,6 +116,7 @@<br>
> PPS m_pps;<br>
> NALList m_nalList;<br>
> ScalingList m_scalingList; // quantization matrix information<br>
> + int m_numThreadLocalData;<br>
><br>
> int m_lastBPSEI;<br>
> uint32_t m_numDelayedPic;<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/frameencoder.cpp<br>
> --- a/source/encoder/frameencoder.cpp Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/frameencoder.cpp Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -45,6 +45,7 @@<br>
> , m_frame(NULL)<br>
> {<br>
> m_totalTime = 0;<br>
> + m_frameEncoderID = 0;<br>
> m_bAllRowsStop = false;<br>
> m_vbvResetTriggerRow = -1;<br>
> m_outStreams = NULL;<br>
> @@ -156,6 +157,7 @@<br>
> PPAScopeEvent(FrameEncoder_compressFrame);<br>
> int64_t startCompressTime = x265_mdate();<br>
> Slice* slice = m_frame->m_picSym->m_slice;<br>
> + m_frame->m_frameEncoderID = m_frameEncoderID; // Each Frame knows the ID of the FrameEncoder encoding it<br>
><br>
> /* Emit access unit delimiter unless this is the first frame and the user is<br>
> * not repeating headers (since AUD is supposed to be the first NAL in the access<br>
> @@ -393,8 +395,36 @@<br>
> if (m_top->m_rateControl->rateControlEnd(m_frame, m_accessUnitBits, &m_rce, &m_frameStats) < 0)<br>
> m_top->m_aborted = true;<br>
><br>
> + /* Accumulate NR statistics from all worker threads */<br>
> + if (m_nr)<br>
> + {<br>
> + for (int i = 0; i < m_top->m_numThreadLocalData; i++)<br>
> + {<br>
> + NoiseReduction* nr = &m_top->m_threadLocalData[i].m_nr[m_frameEncoderID];<br>
> + for (int cat = 0; cat < MAX_NUM_TR_CATEGORIES; cat++)<br>
> + {<br>
> + for(int coeff = 0; coeff < MAX_NUM_TR_COEFFS; coeff++)<br>
> + m_nr->residualSum[cat][coeff] += nr->residualSum[cat][coeff];<br>
> +<br>
> + m_nr->count[cat] += nr->count[cat];<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> noiseReductionUpdate();<br>
><br>
> + /* Copy updated NR coefficients back to all worker threads */<br>
> + if (m_nr)<br>
> + {<br>
> + for (int i = 0; i < m_top->m_numThreadLocalData; i++)<br>
> + {<br>
> + NoiseReduction* nr = &m_top->m_threadLocalData[i].m_nr[m_frameEncoderID];<br>
> + memcpy(nr->offsetDenoise, m_nr->offsetDenoise, sizeof(uint32_t) * MAX_NUM_TR_CATEGORIES * MAX_NUM_TR_COEFFS);<br>
> + memset(nr->count, 0, sizeof(uint32_t) * MAX_NUM_TR_CATEGORIES);<br>
> + memset(nr->residualSum, 0, sizeof(uint32_t) * MAX_NUM_TR_CATEGORIES * MAX_NUM_TR_COEFFS);<br>
> + }<br>
> + }<br>
<br>
</div></div>someone needs to initialize the NR structs in ThreadLocalData when they<br>
are allocated. that's probably where the non-determinism is coming from<br>
<span><br>
> // Decrement referenced frame reference counts, allow them to be recycled<br>
> for (int l = 0; l < numPredDir; l++)<br>
> {<br>
> @@ -616,7 +646,8 @@<br>
> // setup thread-local data<br>
> Slice *slice = m_frame->m_picSym->m_slice;<br>
> TComPicYuv* fenc = m_frame->getPicYuvOrg();<br>
> - tld.analysis.m_quant.m_nr = m_nr;<br>
> + if(m_param->noiseReduction)<br>
<br>
</span>w-s<br>
<span><br>
> + tld.analysis.m_quant.m_nr = &tld.m_nr[m_frameEncoderID];<br>
> tld.analysis.m_me.setSourcePlane(fenc->getLumaAddr(), fenc->getStride());<br>
> tld.analysis.m_log = &tld.analysis.m_sliceTypeLog[m_frame->m_picSym->m_slice->m_sliceType];<br>
> tld.analysis.setQP(slice, slice->m_sliceQp);<br>
> diff -r 38b5733cc629 -r c6e786dbbfaa source/encoder/frameencoder.h<br>
> --- a/source/encoder/frameencoder.h Tue Oct 14 14:35:30 2014 +0530<br>
> +++ b/source/encoder/frameencoder.h Tue Oct 14 14:53:24 2014 +0530<br>
> @@ -148,7 +148,7 @@<br>
> int m_filterRowDelayCus;<br>
> Event m_completionEvent;<br>
> int64_t m_totalTime;<br>
> -<br>
> + int m_frameEncoderID;<br>
> protected:<br>
<br>
</span>w-s, try to keep the blank line before protected:<br>
<span><br>
><br>
> /* analyze / compress frame, can be run in parallel within reference constraints */<br>
</span><span>> _______________________________________________<br>
> x265-devel mailing list<br>
> <a href="mailto:x265-devel@videolan.org" target="_blank">x265-devel@videolan.org</a><br>
> <a href="https://mailman.videolan.org/listinfo/x265-devel" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
<br>
</span><span><font color="#888888">--<br>
Steve Borho<br>
</font></span><div><div>_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org" target="_blank">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>