<div dir="auto">Hi Rob,<div dir="auto"><br></div><div dir="auto">Can you share the test cases and environment set up details as soon as possible where you observed the issue that could be helpful to verify your patches ?</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 8 Jun, 2022, 22:14 Rob Arrow, <<a href="mailto:rob.arrow@v-nova.com">rob.arrow@v-nova.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0);background-color:rgb(255,255,255)">
<span style="color:black;font-size:12pt;background-color:white"><span>Hi,</span></span></div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0);background-color:rgb(255,255,255)">
<div style="color:black;font-size:12pt;background-color:white"><span><br>
</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span>Sorry if you've already received this, I originally submitted the following contribution to <a href="mailto:x265contributions@multicorewareinc.com" target="_blank" rel="noreferrer">x265contributions@multicorewareinc.com</a>, but I received no response so am trying again here.</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span><br>
</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span>Thanks,</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span>Rob</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span><br>
</span><span>
<div title="Profile picture of x265contributions@multicorewareinc.com">
<div>
<div>
<div><span title="x265contributions@multicorewareinc.com">~~~<br>
</span></div>
</div>
</div>
</div>
</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span><br>
</span></div>
<div style="color:black;font-size:12pt;background-color:white"><span>I have identified a deadlock which occurs occasionally with x265. Unfortunately, the deadlock is difficult to reproduce reliably so I am unable to provide steps to reproduce. I have
included a patch in the body below to resolve the deadlock and a description of the conditions leading to the deadlock is included the commit message for the patch.</span></div>
<div style="color:black;font-size:12pt;background-color:white">
<div><br>
</div>
<div>During the investigation of this bug, it was also revealed that there were unprotected mutations of `CTURow.active` and `CTURow.busy`. The proposed patch adds protection to these mutations by taking a `ScopedLock` before accessing them.</div>
</div>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0);background-color:rgb(255,255,255)">
<br>
<p style="FONT-SIZE:10pt;FONT-FAMILY:ARIAL" align="justify"><span style="FONT-FAMILY:Calibri"><span></span></span>
</p><table style="HEIGHT:257px;WIDTH:820px">
<tbody>
<tr>
<td style="FONT-SIZE:10pt;HEIGHT:37px;FONT-FAMILY:Arial;WIDTH:160px" valign="top" rowspan="2" align="center">
<p style="FONT-SIZE:10pt;FONT-FAMILY:Arial" align="center"><span style="FONT-SIZE:8pt"><a title="" href="http://www.v-nova.com/" target="_blank" rel="noreferrer"><img style="HEIGHT:130px;WIDTH:120px" border="0" src="cid:thumbnail_bfa8ec31-1546-45d1-990c-0de39ea526e7.jfif" width="120" height="130"></a><a title="" href="https://www.v-nova.com/perseus-video-compression-technology/#whatis" target="_blank" rel="noreferrer"></a></span></p><span style="FONT-FAMILY:Arial"></span></td>
<td style="FONT-SIZE:10pt;HEIGHT:25px;FONT-FAMILY:Arial;WIDTH:500px" valign="top">
<p style="FONT-SIZE:10pt;FONT-FAMILY:Arial" align="left"><span style="FONT-FAMILY:Calibri"><span style="FONT-SIZE:11pt"><font size="2"><strong><span style="FONT-SIZE:12pt">Rob Arrow</span><br style="FONT-SIZE:11pt"></strong><span style="FONT-SIZE:11pt">Software Engineer<br style="FONT-SIZE:11pt"> <br style="FONT-SIZE:11pt"> <span style="FONT-SIZE:11pt;FONT-FAMILY:Calibri"></span><span style="FONT-FAMILY:Calibri"></span><span style="FONT-SIZE:11pt"></span><span style="FONT-FAMILY:Calibri"></span><span style="FONT-SIZE:11pt"></span></span></font></span></span></p></td></tr>
<tr>
<td style="FONT-SIZE:10pt;HEIGHT:25px;FONT-FAMILY:Arial;WIDTH:660px" valign="top">
<p style="FONT-SIZE:10pt;FONT-FAMILY:Arial"><span style="FONT-FAMILY:Calibri"><span><font color="#828282">20 Eastbourne Terrace <br></font></span></span><span style="FONT-FAMILY:Calibri"><span><font color="#828282">London W2 6LG</font></span></span><span style="FONT-FAMILY:Calibri"><span><br><a href="http://www.v-nova.com" target="_blank" rel="noreferrer">www.v-nova.com<span style="FONT-FAMILY:Calibri"></span></a> <br><br></span></span><span style="FONT-FAMILY:Calibri"><span><span style="FONT-FAMILY:Calibri"><a href="https://www.linkedin.com/company/2760764/admin/" target="_blank" rel="noreferrer"><img border="0" src="cid:adrianaslinkedin_1ea25bda-8391-4a31-8dce-d6ca3ce29f1f.png"></a> <a title="" href="https://twitter.com/VNovaVideo" target="_blank" rel="noreferrer"><img border="0" src="cid:AdrianaTwitter_5893815c-bebb-47eb-adf5-461be8d5de73.png"></a></span></span></span></p></td></tr></tbody></table>
<p style="FONT-SIZE:10pt;MARGIN-BOTTOM:5pt;FONT-FAMILY:Arial;MARGIN-TOP:0px">
<p style="FONT-SIZE:10pt;FONT-FAMILY:ARIAL"><span style="FONT-SIZE:8pt;FONT-FAMILY:"Arial",sans-serif;COLOR:rgb(166,166,166)">This
email and any attachments are sent in strictest confidence for the sole use of
the addressee and may contain legally privileged, confidential, proprietary and
personal data. If you are not the intended recipient, please advise the sender
by replying promptly to this email and then delete and destroy this email and
any attachments without any further use, copying or forwarding. <br><br>For more
information on how we use and protect your information, please review our
Privacy Policy <a href="https://www.v-nova.com/privacy-policy/" target="_blank" rel="noreferrer">here</a></span><span style="FONT-SIZE:8pt;FONT-FAMILY:"Arial",sans-serif;COLOR:rgb(166,166,166)"><br> <br></span><span style="FONT-SIZE:8pt;FONT-FAMILY:"Arial",sans-serif;COLOR:rgb(0,57,115)">Please
consider your environmental responsibility before printing this
e-mail</span></p></p>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0);background-color:rgb(255,255,255)">
<div style="background-color:white;color:black">
<div>
<div>
<div dir="ltr">
<div style="color:black;font-size:12pt;background-color:white">
<div><span>From: Rob Arrow <<a href="mailto:rob.arrow@v-nova.com" target="_blank" rel="noreferrer">rob.arrow@v-nova.com</a>></span>
<div>Date: Thu, 16 Dec 2021 13:30:20 +0000</div>
<span>Subject: [PATCH 1/2] Protect access to CTURow.active CTURow.busy</span><br>
</div>
<div><br>
</div>
<div><span>The comments in frameencoder.h state that the CTURow.lock "must be</span>
<div>acquired when reading or writing m_active or m_busy". "Previously</div>
<div>there were two points in the code where CTURow.active and CTURow.busy</div>
<div>were being modified without being protected by the CTURow.lock mutex.</div>
<div>This commit protects the modifications by taking the lock.</div>
<div>---</div>
<div> source/encoder/frameencoder.cpp | 9 +++++++--</div>
<div> 1 file changed, 7 insertions(+), 2 deletions(-)</div>
<div><br>
</div>
<div>diff --git a/source/encoder/frameencoder.cpp b/source/encoder/frameencoder.cpp</div>
<div>index 1b3875a25..e0c3c55b8 100644</div>
<div>--- a/source/encoder/frameencoder.cpp</div>
<div>+++ b/source/encoder/frameencoder.cpp</div>
<div>@@ -818,8 +818,12 @@ void FrameEncoder::compressFrame()</div>
<div> * compressed in a wave-front pattern if WPP is enabled. Row based loop</div>
<div> * filters runs behind the CTU compression and reconstruction */</div>
<div> </div>
<div>- for (uint32_t sliceId = 0; sliceId < m_param->maxSlices; sliceId++) </div>
<div>- m_rows[m_sliceBaseRow[sliceId]].active = true;</div>
<div>+ for (uint32_t sliceId = 0; sliceId < m_param->maxSlices; sliceId++)</div>
<div>+ {</div>
<div>+ CTURow& curRow = m_rows[m_sliceBaseRow[sliceId]];</div>
<div>+ ScopedLock self(curRow.lock);</div>
<div>+ curRow.active = true;</div>
<div>+ }</div>
<div> </div>
<div> if (m_param->bEnableWavefront)</div>
<div> {</div>
<div>@@ -1909,6 +1913,7 @@ void FrameEncoder::processRowEncoder(int intRow, ThreadLocalData& tld)</div>
<div> }</div>
<div> }</div>
<div> </div>
<div>+ ScopedLock self(curRow.lock);</div>
<div> curRow.busy = false;</div>
<div> </div>
<div> // CHECK_ME: Does it always FALSE condition?</div>
<div>-- </div>
<div>2.25.1</div>
<div><br>
</div>
<div>From caaf4d9d4ee86cf5479b47cbc0e94dca3fe626a1 Mon Sep 17 00:00:00 2001</div>
</div>
</div>
</div>
</div>
</div>
</div>
<div>
<div>
<div>
<div dir="ltr">
<div style="color:black;font-size:12pt;background-color:white">
<div>
<div>From: Rob Arrow <<a href="mailto:rob.arrow@v-nova.com" target="_blank" rel="noreferrer">rob.arrow@v-nova.com</a>></div>
<div>Date: Thu, 16 Dec 2021 13:33:58 +0000</div>
<div>Subject: [PATCH 2/2] Prevent VBV underflow deadlock</div>
<div><br>
</div>
<div>Previously, an issue could occur within `frameencoder.cpp` when a VBV underflow was detected that requires the encode to be restarted in one thread (thread A). If the last row of the slice has been marked enqueued and completed all of the CUs in another
thread (thread B) before thread A sets m_vbvResetTriggerRow and m_bAllRowsStop, it is possible for a deadlock to occur within thread A this loop in `frameencoder.cpp`:</div>
<div><br>
</div>
<div>```</div>
<div>while (stopRow.active)</div>
<div>{</div>
<div> if (dequeueRow(m_row_to_idx[r] * 2))</div>
<div> stopRow.active = false;</div>
<div> else</div>
<div> {</div>
<div> /* we must release the row lock to allow the thread to exit */</div>
<div> stopRow.lock.release();</div>
<div> GIVE_UP_TIME();</div>
<div> stopRow.lock.acquire();</div>
<div> }</div>
<div>}</div>
<div>```</div>
<div><br>
</div>
<div>In this case, stopRow.active was set by the row above in thread B. The only way for thread A to exit the while loop is for `stopRow.active` to be cleared elsewhere or for `dequeueRow()` to return true. `dequeueRow()` is unable to return true as thead B
has already removed the row from the queue when it was processed by `WaveFront::findJob()`.</div>
<div><br>
</div>
<div>There was previously no code path available for `stopRow.active` to be cleared in another thread to prevent this deadlock occurring.</div>
<div><br>
</div>
<div>This commit clears the active variable when the row is completed at the end of `FrameEncoder::processRowEncoder()` in thread B.</div>
<div>---</div>
<div> source/encoder/frameencoder.cpp | 1 +</div>
<div> 1 file changed, 1 insertion(+)</div>
<div><br>
</div>
<div>diff --git a/source/encoder/frameencoder.cpp b/source/encoder/frameencoder.cpp</div>
<div>index e0c3c55b8..3ea281da1 100644</div>
<div>--- a/source/encoder/frameencoder.cpp</div>
<div>+++ b/source/encoder/frameencoder.cpp</div>
<div>@@ -1915,6 +1915,7 @@ void FrameEncoder::processRowEncoder(int intRow, ThreadLocalData& tld)</div>
<div> </div>
<div> ScopedLock self(curRow.lock);</div>
<div> curRow.busy = false;</div>
<div>+ curRow.active = false;</div>
<div> </div>
<div> // CHECK_ME: Does it always FALSE condition?</div>
<div> if (ATOMIC_INC(&m_completionCount) == 2 * (int)m_numRows)</div>
<div>-- </div>
<span>2.25.1</span> </div>
</div>
</div>
</div>
</div>
</div>
<br>
</div>
</div>
_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org" target="_blank" rel="noreferrer">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" rel="noreferrer noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div>