[x265] x265 Intermittent VBV Underflow Deadlock Patch
Rob Arrow
rob.arrow at v-nova.com
Wed Jun 8 16:43:54 UTC 2022
Hi,
Sorry if you've already received this, I originally submitted the following contribution to x265contributions at 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.arrow at 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.arrow at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220608/f53fb926/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thumbnail_bfa8ec31-1546-45d1-990c-0de39ea526e7.jfif
Type: application/octet-stream
Size: 40523 bytes
Desc: thumbnail_bfa8ec31-1546-45d1-990c-0de39ea526e7.jfif
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220608/f53fb926/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adrianaslinkedin_1ea25bda-8391-4a31-8dce-d6ca3ce29f1f.png
Type: image/png
Size: 1488 bytes
Desc: adrianaslinkedin_1ea25bda-8391-4a31-8dce-d6ca3ce29f1f.png
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220608/f53fb926/attachment-0002.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AdrianaTwitter_5893815c-bebb-47eb-adf5-461be8d5de73.png
Type: image/png
Size: 1471 bytes
Desc: AdrianaTwitter_5893815c-bebb-47eb-adf5-461be8d5de73.png
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220608/f53fb926/attachment-0003.png>
More information about the x265-devel
mailing list