<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 12, 2020 at 3:55 PM Srikanth Kurapati <<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"><div dir="ltr"><br><div>Addressed all the comments.  </div><div>I would like to understand the reason for computing the edge histogram on pixel depth. Why can't we do it on source depth?   <br></div><div>Ans: edge pixel range is determined by x265 build bit-depth and not source depth and all pictures are modified to match with the x265 build bit depth and not the source depth.</div></div></blockquote><div><br></div><div>I understand this. But why is the edge info computed on internal pixel depth? Do you any accuracy difference in computing edge info for internal bit depth? </div><div>I see this depth conversion as a redundant step here as it is done again during picture to frame conversion phase. I would also like to understand the performance impact of this conversion.</div><div>Please clarify.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 12, 2020 at 12:24 PM Aruna Matheswaran <<a href="mailto:aruna@multicorewareinc.com" target="_blank">aruna@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"><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>
_______________________________________________<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"><b style="background-color:rgb(255,255,255)"><font color="#0b5394">With Regards,</font></b><div><b style="background-color:rgb(255,255,255)"><font color="#0b5394">Srikanth Kurapati.</font></b></div></div></div>
_______________________________________________<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>