<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 11, 2020 at 12:01 PM <<a href="mailto:srikanth.kurapati@multicorewareinc.com" target="_blank">srikanth.kurapati@multicorewareinc.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"># HG changeset patch<br>
# User Srikanth Kurapati<br>
# Date 1580113966 -19800<br>
# Mon Jan 27 14:02:46 2020 +0530<br>
# Node ID a542cabea72ac65f8e19da1bd2e5e75c871d3d42<br>
# Parent 30d303b38c7bd1733aedd01a3f738fb08ec1488c<br>
Fix: Segmentation fault for hist-scenecut option in 16bpp builds.<br>
<br>
This patch fixes segmentation fault due to bit depth mismatch between source<br>
input and x265 builds encountered for hist-scenecut feature.<br></blockquote><div><br></div><div>The patch not just fixes the segfault in high bit-depth encodes, but also fixes incorrect edge computation that was happening earlier with 10bit source and 8bpp builds.</div><div>Please clarify the same in the commit message.</div><div><br></div><div>Also, I would like to understand the reason for computing the edge histogram on pixel depth. Why can't we do it on source depth? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
diff -r 30d303b38c7b -r a542cabea72a source/common/common.h<br>
--- a/source/common/common.h Wed Jan 29 12:19:07 2020 +0530<br>
+++ b/source/common/common.h Mon Jan 27 14:02:46 2020 +0530<br>
@@ -131,7 +131,6 @@<br>
typedef int64_t ssum2_t;<br>
#define SHIFT_TO_BITPLANE 9<br>
#define HISTOGRAM_BINS 1024<br>
-#define SHIFT 1<br>
#else<br>
typedef uint8_t pixel;<br>
typedef uint16_t sum_t;<br>
@@ -140,7 +139,6 @@<br>
typedef int32_t ssum2_t; // Signed sum<br>
#define SHIFT_TO_BITPLANE 7<br>
#define HISTOGRAM_BINS 256<br>
-#define SHIFT 0<br>
#endif // if HIGH_BIT_DEPTH<br>
<br>
#if X265_DEPTH < 10<br>
diff -r 30d303b38c7b -r a542cabea72a source/encoder/encoder.cpp<br>
--- a/source/encoder/encoder.cpp Wed Jan 29 12:19:07 2020 +0530<br>
+++ b/source/encoder/encoder.cpp Mon Jan 27 14:02:46 2020 +0530<br>
@@ -220,9 +220,9 @@<br>
{<br>
for (int i = 0; i < x265_cli_csps[m_param->internalCsp].planes; i++)<br>
{<br>
- m_planeSizes[i] = m_param->sourceWidth * m_param->sourceHeight >> x265_cli_csps[m_param->internalCsp].height[i];<br>
- }<br>
- uint32_t pixelbytes = m_param->sourceBitDepth > 8 ? 2 : 1;<br>
+ m_planeSizes[i] = (m_param->sourceWidth >> x265_cli_csps[p->internalCsp].width[i]) * (m_param->sourceHeight >> x265_cli_csps[m_param->internalCsp].height[i]);<br>
+ }<br>
+ uint32_t pixelbytes = m_param->internalBitDepth > 8 ? 2 : 1;<br>
m_edgePic = X265_MALLOC(pixel, m_planeSizes[0] * pixelbytes);<br>
m_edgeHistThreshold = m_param->edgeTransitionThreshold;<br>
m_chromaHistThreshold = m_edgeHistThreshold * 10.0;<br>
@@ -231,6 +231,23 @@<br>
m_scaledEdgeThreshold = x265_min(m_scaledEdgeThreshold, MAX_SCENECUT_THRESHOLD);<br>
m_scaledChromaThreshold = m_chromaHistThreshold * SCENECUT_STRENGTH_FACTOR;<br>
m_scaledChromaThreshold = x265_min(m_scaledChromaThreshold, MAX_SCENECUT_THRESHOLD);<br>
+ if (!(m_param->sourceBitDepth == 8 && m_param->internalBitDepth == 8))<br></blockquote><div><br></div><div>This extra memory allocation for plane buffers is required only if the source picture depth and build depth differs. Why does this check compare them to 8-bit always?</div><div>This would allocate memory unnecessarily for 10bpp builds with 10bit source.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ {<br>
+ int size = m_param->sourceWidth * m_param->sourceHeight;<br>
+ int hshift = CHROMA_H_SHIFT(m_param->internalCsp);<br>
+ int vshift = CHROMA_V_SHIFT(m_param->internalCsp);<br>
+ int widthC = m_param->sourceWidth >> hshift;<br>
+ int heightC = m_param->sourceHeight >> vshift;<br>
+<br>
+ m_inputPic[0] = X265_MALLOC(pixel, size);<br>
+ if (m_param->internalCsp != X265_CSP_I400)<br>
+ {<br>
+ for (int j = 1; j < 3; j++)<br>
+ {<br>
+ m_inputPic[j] = X265_MALLOC(pixel, widthC * heightC);<br>
+ }<br>
+ }<br>
+ }<br>
}<br>
<br>
// Do not allow WPP if only one row or fewer than 3 columns, it is pointless and unstable<br>
@@ -874,6 +891,21 @@<br>
{<br>
X265_FREE_ZERO(m_edgePic);<br>
}<br>
+<br>
+ if (!(m_param->sourceBitDepth == 8 && m_param->internalBitDepth == 8))<br>
+ {<br></blockquote><div>Same here. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ for (int i = 0; i < 3; i++)<br>
+ {<br>
+ if (i == 0)<br>
+ {<br>
+ X265_FREE_ZERO(m_inputPic[i]);<br>
+ }<br>
+ else if (i >= 1 && m_param->internalCsp != X265_CSP_I400)<br>
+ {<br></blockquote><div>This condition needs to be optimized. "i >= 1" is unwanted. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ X265_FREE_ZERO(m_inputPic[i]);<br>
+ }<br>
+ }<br>
+ }<br>
}<br>
<br>
for (int i = 0; i < m_param->frameNumThreads; i++)<br>
@@ -1337,11 +1369,87 @@<br>
<br>
bool Encoder::computeHistograms(x265_picture *pic)<br>
{<br>
- pixel *src = (pixel *) pic->planes[0];<br>
+ pixel *src = NULL, *planeV = NULL, *planeU = NULL;<br>
+ uint32_t widthC, heightC;<br>
+ int hshift, vshift;<br>
+<br>
+ hshift = CHROMA_H_SHIFT(pic->colorSpace);<br>
+ vshift = CHROMA_V_SHIFT(pic->colorSpace);<br>
+ widthC = pic->width >> hshift;<br>
+ heightC = pic->height >> vshift;<br>
+<br>
+ if (pic->bitDepth == 8 && X265_DEPTH == 8)<br>
+ {<br>
+ src = (pixel*)pic->planes[0];<br>
+ if (m_param->internalCsp != X265_CSP_I400)<br>
+ {<br>
+ planeU = (pixel*)pic->planes[1];<br>
+ planeV = (pixel*)pic->planes[2];<br>
+ }<br>
+ }<br>
+ else if (pic->bitDepth == 8 && X265_DEPTH > 8)<br>
+ {<br>
+ int shift = (X265_DEPTH - 8);<br>
+ uint8_t *yChar, *uChar, *vChar;<br>
+<br>
+ yChar = (uint8_t*)pic->planes[0];<br>
+ primitives.planecopy_cp(yChar, pic->stride[0] / sizeof(*yChar), m_inputPic[0], pic->stride[0] / sizeof(*yChar), pic->width, pic->height, shift);<br>
+<br>
+ if (m_param->internalCsp != X265_CSP_I400)<br>
+ {<br>
+ uChar = (uint8_t*)pic->planes[1];<br>
+ vChar = (uint8_t*)pic->planes[2];<br>
+ primitives.planecopy_cp(uChar, pic->stride[1] / sizeof(*uChar), m_inputPic[1], pic->stride[1] / sizeof(*uChar), widthC, heightC, shift);<br>
+ primitives.planecopy_cp(vChar, pic->stride[2] / sizeof(*vChar), m_inputPic[2], pic->stride[2] / sizeof(*vChar), widthC, heightC, shift);<br>
+ }<br>
+ }<br>
+ else<br>
+ {<br>
+ uint16_t *yShort, *uShort, *vShort;<br>
+ /* mask off bits that are supposed to be zero */<br>
+ uint16_t mask = (1 << X265_DEPTH) - 1;<br>
+ int shift = abs(pic->bitDepth - X265_DEPTH);<br></blockquote><div>Again, this shifting is unnecessary for 10bit builds + 10bit source. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ yShort = (uint16_t*)pic->planes[0];<br>
+ if (pic->bitDepth > X265_DEPTH)<br>
+ {<br>
+ /* shift right and mask pixels to final size */<br>
+ primitives.planecopy_sp(yShort, pic->stride[0] / sizeof(*yShort), m_inputPic[0], pic->stride[0] / sizeof(*yShort), pic->width, pic->height, shift, mask);<br>
+ }<br>
+ else /* Case for (pic.bitDepth <= X265_DEPTH) */<br>
+ {<br>
+ /* shift left and mask pixels to final size */<br>
+ primitives.planecopy_sp_shl(yShort, pic->stride[0] / sizeof(*yShort), m_inputPic[0], pic->stride[0] / sizeof(*yShort), pic->width, pic->height, shift, mask);<br>
+ }<br>
+<br>
+ if (m_param->internalCsp != X265_CSP_I400)<br>
+ {<br>
+ uShort = (uint16_t*)pic->planes[1];<br>
+ vShort = (uint16_t*)pic->planes[2];<br>
+<br>
+ if (pic->bitDepth > X265_DEPTH)<br>
+ {<br>
+ primitives.planecopy_sp(uShort, pic->stride[1] / sizeof(*uShort), m_inputPic[1], pic->stride[1] / sizeof(*uShort), widthC, heightC, shift, mask);<br>
+ primitives.planecopy_sp(vShort, pic->stride[2] / sizeof(*vShort), m_inputPic[2], pic->stride[2] / sizeof(*vShort), widthC, heightC, shift, mask);<br>
+ }<br>
+ else /* Case for (pic.bitDepth <= X265_DEPTH) */<br>
+ {<br>
+ primitives.planecopy_sp_shl(uShort, pic->stride[1] / sizeof(*uShort), m_inputPic[1], pic->stride[1] / sizeof(*uShort), widthC, heightC, shift, mask);<br>
+ primitives.planecopy_sp_shl(vShort, pic->stride[2] / sizeof(*vShort), m_inputPic[2], pic->stride[2] / sizeof(*vShort), widthC, heightC, shift, mask);<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ if (!(pic->bitDepth == 8 && X265_DEPTH == 8))<br>
+ {<br>
+ src = m_inputPic[0];<br>
+ planeU = m_inputPic[1];<br>
+ planeV = m_inputPic[2];<br></blockquote><div>Redundant code. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+<br>
size_t bufSize = sizeof(pixel) * m_planeSizes[0];<br>
int32_t planeCount = x265_cli_csps[m_param->internalCsp].planes;<br>
- int32_t numBytes = m_param->sourceBitDepth > 8 ? 2 : 1;<br>
- memset(m_edgePic, 0, bufSize * numBytes);<br>
+ memset(m_edgePic, 0, bufSize);<br>
<br>
if (!computeEdge(m_edgePic, src, NULL, pic->width, pic->height, pic->width, false, 1))<br>
{<br>
@@ -1350,10 +1458,9 @@<br>
}<br>
<br>
pixel pixelVal;<br>
- int64_t size = pic->height * (pic->stride[0] >> SHIFT);<br>
int32_t *edgeHist = m_curEdgeHist;<br>
memset(edgeHist, 0, 2 * sizeof(int32_t));<br>
- for (int64_t i = 0; i < size; i++)<br>
+ for (int64_t i = 0; i < m_planeSizes[0]; i++)<br>
{<br>
if (!m_edgePic[i])<br>
edgeHist[0]++;<br>
@@ -1364,16 +1471,12 @@<br>
if (pic->colorSpace != X265_CSP_I400)<br>
{<br>
/* U Histogram Calculation */<br>
- int32_t HeightL = (pic->height >> x265_cli_csps[pic->colorSpace].height[1]);<br>
- size = HeightL * (pic->stride[1] >> SHIFT);<br>
int32_t *uHist = m_curUVHist[0];<br>
- pixel *chromaPlane = (pixel *) pic->planes[1];<br>
-<br>
memset(uHist, 0, HISTOGRAM_BINS * sizeof(int32_t));<br>
<br>
- for (int64_t i = 0; i < size; i++)<br>
- {<br>
- pixelVal = chromaPlane[i];<br>
+ for (int64_t i = 0; i < m_planeSizes[1]; i++)<br>
+ {<br>
+ pixelVal = planeU[i];<br>
uHist[pixelVal]++;<br>
}<br>
<br>
@@ -1381,15 +1484,12 @@<br>
if (planeCount == 3)<br>
{<br>
pixelVal = 0;<br>
- int32_t heightV = (pic->height >> x265_cli_csps[pic->colorSpace].height[2]);<br>
- size = heightV * (pic->stride[2] >> SHIFT);<br>
int32_t *vHist = m_curUVHist[1];<br>
- chromaPlane = (pixel *) pic->planes[2];<br>
-<br>
memset(vHist, 0, HISTOGRAM_BINS * sizeof(int32_t));<br>
- for (int64_t i = 0; i < size; i++)<br>
+<br>
+ for (int64_t i = 0; i < m_planeSizes[2]; i++)<br>
{<br>
- pixelVal = chromaPlane[i];<br>
+ pixelVal = planeV[i];<br>
vHist[pixelVal]++;<br>
}<br>
for (int i = 0; i < HISTOGRAM_BINS; i++)<br>
diff -r 30d303b38c7b -r a542cabea72a source/encoder/encoder.h<br>
--- a/source/encoder/encoder.h Wed Jan 29 12:19:07 2020 +0530<br>
+++ b/source/encoder/encoder.h Mon Jan 27 14:02:46 2020 +0530<br>
@@ -255,6 +255,7 @@<br>
<br>
/* For histogram based scene-cut detection */<br>
pixel* m_edgePic;<br>
+ pixel* m_inputPic[3];<br>
int32_t m_curUVHist[2][HISTOGRAM_BINS];<br>
int32_t m_curMaxUVHist[HISTOGRAM_BINS];<br>
int32_t m_prevMaxUVHist[HISTOGRAM_BINS];<br>
_______________________________________________<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" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><font face="georgia, serif">Regards,</font><div><b><font face="georgia, serif">Aruna Matheswaran,</font></b></div><div><font face="georgia, serif">Video Codec Engineer,</font></div><div><font face="georgia, serif">Media & AI analytics BU,</font></div><div><span><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;vertical-align:baseline;white-space:pre-wrap"><span style="border:none;display:inline-block;overflow:hidden;width:153px;height:58px"><img src="https://lh5.googleusercontent.com/gjX5cPNIZgwUrhfqkTwQUZWztIKmmo0qs3kbwvkS5H-bDVE2ftte9pMTVnFLSjOcjYWLtfc6_OGpxW4vraLg2r5QAIf1Q3MpldFDgWtzK_gXi8ptw5B3joIbsGL6mxj-JRdjHzT5" width="96" height="36" style="margin-left:0px;margin-top:0px"></span></span></span><font face="georgia, serif"><br></font></div><div><span><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;vertical-align:baseline;white-space:pre-wrap"><span style="border:none;display:inline-block;overflow:hidden;width:153px;height:58px"><img src="https://lh5.googleusercontent.com/gjX5cPNIZgwUrhfqkTwQUZWztIKmmo0qs3kbwvkS5H-bDVE2ftte9pMTVnFLSjOcjYWLtfc6_OGpxW4vraLg2r5QAIf1Q3MpldFDgWtzK_gXi8ptw5B3joIbsGL6mxj-JRdjHzT5" style="margin-left:0px;margin-top:0px"></span></span></span><font face="georgia, serif"><br></font></div><div><font face="georgia, serif"><br></font></div></div></div></div></div></div></div></div></div></div>