[x265] x265 Intermittent VBV Underflow Deadlock Patch
Srikanth Kurapati
srikanth.kurapati at multicorewareinc.com
Mon Jul 25 05:14:33 UTC 2022
Hi Rob,
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 ?
On Wed, 8 Jun, 2022, 22:14 Rob Arrow, <rob.arrow at v-nova.com> wrote:
> 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.
>
> <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
>
> <https://www.linkedin.com/company/2760764/admin/>
> <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
>
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220725/05f854b4/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: not available
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220725/05f854b4/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: not available
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220725/05f854b4/attachment-0003.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AdrianaTwitter_5893815c-bebb-47eb-adf5-461be8d5de73.png
Type: image/png
Size: 1471 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220725/05f854b4/attachment-0004.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adrianaslinkedin_1ea25bda-8391-4a31-8dce-d6ca3ce29f1f.png
Type: image/png
Size: 1488 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20220725/05f854b4/attachment-0005.png>
More information about the x265-devel
mailing list