Hi, Sorry if you've already received this, I originally submitted the following contribution to x265contributi...@multicorewareinc.com, but I received no response so am trying again here.
Thanks, Rob ~~~ 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. 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. [cid:thumbnail_bfa8ec31-1546-45d1-990c-0de39ea526e7.jfif]<http://www.v-nova.com/><https://www.v-nova.com/perseus-video-compression-technology/#whatis> Rob Arrow Software Engineer 20 Eastbourne Terrace London W2 6LG www.v-nova.com<http://www.v-nova.com> [cid:adrianaslinkedin_1ea25bda-8391-4a31-8dce-d6ca3ce29f1f.png]<https://www.linkedin.com/company/2760764/admin/> [cid:AdrianaTwitter_5893815c-bebb-47eb-adf5-461be8d5de73.png] <https://twitter.com/VNovaVideo> 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. For more information on how we use and protect your information, please review our Privacy Policy here<https://www.v-nova.com/privacy-policy/> Please consider your environmental responsibility before printing this e-mail From: Rob Arrow <rob.ar...@v-nova.com> Date: Thu, 16 Dec 2021 13:30:20 +0000 Subject: [PATCH 1/2] Protect access to CTURow.active CTURow.busy The comments in frameencoder.h state that the CTURow.lock "must be acquired when reading or writing m_active or m_busy". "Previously there were two points in the code where CTURow.active and CTURow.busy were being modified without being protected by the CTURow.lock mutex. This commit protects the modifications by taking the lock. --- source/encoder/frameencoder.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/source/encoder/frameencoder.cpp b/source/encoder/frameencoder.cpp index 1b3875a25..e0c3c55b8 100644 --- a/source/encoder/frameencoder.cpp +++ b/source/encoder/frameencoder.cpp @@ -818,8 +818,12 @@ void FrameEncoder::compressFrame() * compressed in a wave-front pattern if WPP is enabled. Row based loop * filters runs behind the CTU compression and reconstruction */ - for (uint32_t sliceId = 0; sliceId < m_param->maxSlices; sliceId++) - m_rows[m_sliceBaseRow[sliceId]].active = true; + for (uint32_t sliceId = 0; sliceId < m_param->maxSlices; sliceId++) + { + CTURow& curRow = m_rows[m_sliceBaseRow[sliceId]]; + ScopedLock self(curRow.lock); + curRow.active = true; + } if (m_param->bEnableWavefront) { @@ -1909,6 +1913,7 @@ void FrameEncoder::processRowEncoder(int intRow, ThreadLocalData& tld) } } + ScopedLock self(curRow.lock); curRow.busy = false; // CHECK_ME: Does it always FALSE condition? -- 2.25.1 >From caaf4d9d4ee86cf5479b47cbc0e94dca3fe626a1 Mon Sep 17 00:00:00 2001 From: Rob Arrow <rob.ar...@v-nova.com> Date: Thu, 16 Dec 2021 13:33:58 +0000 Subject: [PATCH 2/2] Prevent VBV underflow deadlock 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`: ``` while (stopRow.active) { if (dequeueRow(m_row_to_idx[r] * 2)) stopRow.active = false; else { /* we must release the row lock to allow the thread to exit */ stopRow.lock.release(); GIVE_UP_TIME(); stopRow.lock.acquire(); } } ``` 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()`. There was previously no code path available for `stopRow.active` to be cleared in another thread to prevent this deadlock occurring. This commit clears the active variable when the row is completed at the end of `FrameEncoder::processRowEncoder()` in thread B. --- source/encoder/frameencoder.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/source/encoder/frameencoder.cpp b/source/encoder/frameencoder.cpp index e0c3c55b8..3ea281da1 100644 --- a/source/encoder/frameencoder.cpp +++ b/source/encoder/frameencoder.cpp @@ -1915,6 +1915,7 @@ void FrameEncoder::processRowEncoder(int intRow, ThreadLocalData& tld) ScopedLock self(curRow.lock); curRow.busy = false; + curRow.active = false; // CHECK_ME: Does it always FALSE condition? if (ATOMIC_INC(&m_completionCount) == 2 * (int)m_numRows) -- 2.25.1
_______________________________________________ x265-devel mailing list x265-devel@videolan.org https://mailman.videolan.org/listinfo/x265-devel
