[x265] [PATCH REVIEW] cli: add cli option max-tu-size and support for it

[Steve Borho]

On 02/10, santhoshini at multicorewareinc.com wrote:
> # HG changeset patch
> # User Santhoshini Sekar<santhoshini at multicorewareinc.com>
> # Date 1423547171 -19800
> #      Tue Feb 10 11:16:11 2015 +0530
> # Node ID 3a32b6a482b38c1c87bc690fbef2c39d32da5094
> # Parent  da3302cc67fb06c62726d175593fdfda0db2fd54
> cli: add cli option max-tu-size and support for it

Looks reasonable, just a few nits..

> diff -r da3302cc67fb -r 3a32b6a482b3 source/common/param.cpp
> --- a/source/common/param.cpp	Mon Feb 09 16:45:31 2015 -0600
> +++ b/source/common/param.cpp	Tue Feb 10 11:16:11 2015 +0530
> @@ -129,6 +129,7 @@
>      param->maxCUSize = 64;
>      param->tuQTMaxInterDepth = 1;
>      param->tuQTMaxIntraDepth = 1;
> +    param->maxTUSize = 32;
>  
>      /* Coding Structure */
>      param->keyframeMin = 0;
> @@ -572,6 +573,7 @@
>      OPT("ctu") p->maxCUSize = (uint32_t)atoi(value);
>      OPT("tu-intra-depth") p->tuQTMaxIntraDepth = (uint32_t)atoi(value);
>      OPT("tu-inter-depth") p->tuQTMaxInterDepth = (uint32_t)atoi(value);
> +    OPT("max-tu-size") p->maxTUSize = (uint32_t)atoi(value);
>      OPT("subme") p->subpelRefine = atoi(value);
>      OPT("merange") p->searchRange = atoi(value);
>      OPT("rect") p->bEnableRectInter = atobool(value);
> @@ -1012,7 +1014,8 @@
>            "QuadtreeTUMaxDepthIntra must be greater 0 and less than 5");
>      CHECK(maxLog2CUSize < tuQTMinLog2Size + param->tuQTMaxIntraDepth - 1,
>            "QuadtreeTUMaxDepthInter must be less than or equal to the difference between log2(maxCUSize) and QuadtreeTULog2MinSize plus 1");
> -
> +    CHECK((param->maxTUSize != 32 && param->maxTUSize != 16 && param->maxTUSize != 8 && param->maxTUSize != 4) || param->maxTUSize > param->maxCUSize,
> +          "max TU size must be 4, 8, 16, or 32 and should be less than max CU size");
>      CHECK(param->maxNumMergeCand < 1, "MaxNumMergeCand must be 1 or greater.");
>      CHECK(param->maxNumMergeCand > 5, "MaxNumMergeCand must be 5 or smaller.");
>  
> @@ -1194,8 +1197,8 @@
>      if (param->interlaceMode)
>          x265_log(param, X265_LOG_INFO, "Interlaced field inputs             : %s\n", x265_interlace_names[param->interlaceMode]);
>  
> -    x265_log(param, X265_LOG_INFO, 
> -             param->maxCUSize, param->tuQTMaxInterDepth, param->tuQTMaxIntraDepth);
> +    x265_log(param, X265_LOG_INFO, "CTU size / TU size / RQT depth inter / intra  : %d / %d / %d / %d\n",
> +        param->maxCUSize, param->maxTUSize, param->tuQTMaxInterDepth, param->tuQTMaxIntraDepth);

This interferes with the alignment of the colons in the param logs. How
about if we split this into two lines:

  "Coding QT: max CU size, min CU size : %d / %d\n", param->maxCUSize, 8);
  "Residual QT: max TU size, max depth : %d / %d inter / %d intra\n",
      param->maxTUSize, param->tuQTMaxInterDepth, param->tuQTMaxIntraDepth);

Then when we make min CU size adjustable there will be a place to log
it.

>      x265_log(param, X265_LOG_INFO, "ME / range / subpel / merge         : %s / %d / %d / %d\n",
>               x265_motion_est_names[param->searchMethod], param->searchRange, param->subpelRefine, param->maxNumMergeCand);
> @@ -1291,6 +1294,7 @@
>      s += sprintf(s, " bitdepth=%d", p->internalBitDepth);
>      BOOL(p->bEnableWavefront, "wpp");
>      s += sprintf(s, " ctu=%d", p->maxCUSize);
> +    s += sprintf(s, " max-tu-size=%d", p->maxTUSize);
>      s += sprintf(s, " tu-intra-depth=%d", p->tuQTMaxIntraDepth);
>      s += sprintf(s, " tu-inter-depth=%d", p->tuQTMaxInterDepth);
>      s += sprintf(s, " me=%d", p->searchMethod);
> diff -r da3302cc67fb -r 3a32b6a482b3 source/encoder/encoder.cpp
> --- a/source/encoder/encoder.cpp	Mon Feb 09 16:45:31 2015 -0600
> +++ b/source/encoder/encoder.cpp	Tue Feb 10 11:16:11 2015 +0530
> @@ -1438,8 +1438,8 @@
>  
>      sps->log2MinCodingBlockSize = g_maxLog2CUSize - g_maxCUDepth;
>      sps->log2DiffMaxMinCodingBlockSize = g_maxCUDepth;
> -
> -    sps->quadtreeTULog2MaxSize = X265_MIN(g_maxLog2CUSize, 5);
> +    uint32_t maxLog2TUSize = (uint32_t)g_log2Size[m_param->maxTUSize];
> +    sps->quadtreeTULog2MaxSize = X265_MIN(g_maxLog2CUSize, maxLog2TUSize);
>      sps->quadtreeTULog2MinSize = 2;
>      sps->quadtreeTUMaxDepthInter = m_param->tuQTMaxInterDepth;
>      sps->quadtreeTUMaxDepthIntra = m_param->tuQTMaxIntraDepth;
> diff -r da3302cc67fb -r 3a32b6a482b3 source/x265.h
> --- a/source/x265.h	Mon Feb 09 16:45:31 2015 -0600
> +++ b/source/x265.h	Tue Feb 10 11:16:11 2015 +0530
> @@ -507,6 +507,11 @@
>       * compressed by the DCT transforms, at the expense of much more compute */
>      uint32_t  tuQTMaxIntraDepth;
>  
> +    /* Maxiumum TU width and height in pixels.  The size must be 32, 16, 8 or 4.
> +     * The larger the size the more efficiently the residual can be
> +     * compressed by the DCT transforms, at the expense of huge computation */
> +    uint32_t  maxTUSize;

'huge computation' is maybe editorializing a bit too much. 'more
computation' is sufficient.

>      /*== GOP Structure and Lokoahead ==*/
>  
>      /* Enable open GOP - meaning I slices are not necessariy IDR and thus frames
> diff -r da3302cc67fb -r 3a32b6a482b3 source/x265cli.h
> --- a/source/x265cli.h	Mon Feb 09 16:45:31 2015 -0600
> +++ b/source/x265cli.h	Tue Feb 10 11:16:11 2015 +0530
> @@ -71,6 +71,7 @@
>      { "no-wpp",               no_argument, NULL, 0 },
>      { "wpp",                  no_argument, NULL, 0 },
>      { "ctu",            required_argument, NULL, 's' },
> +    { "max-tu-size",    required_argument, NULL, 's' },
>      { "tu-intra-depth", required_argument, NULL, 0 },
>      { "tu-inter-depth", required_argument, NULL, 0 },
>      { "me",             required_argument, NULL, 0 },
> @@ -264,6 +265,7 @@
>      H0("                                 psnr, ssim, grain, zerolatency, fastdecode\n");
>      H0("\nQuad-Tree size and depth:\n");
>      H0("-s/--ctu <64|32|16>              Maximum CU size (WxH). Default %d\n", param->maxCUSize);
> +    H0("-s/--max-tu-size <32|16|8|4>     Maximum TU size (WxH). Default %d\n", param->maxTUSize);
>      H0("   --tu-intra-depth <integer>    Max TU recursive depth for intra CUs. Default %d\n", param->tuQTMaxIntraDepth);
>      H0("   --tu-inter-depth <integer>    Max TU recursive depth for inter CUs. Default %d\n", param->tuQTMaxInterDepth);
>      H0("\nAnalysis:\n");

Whenever a commit changes binary compatibility to the public API (adding
a field to x265_param qualifies) we must bump X265_BUILD in the master
cmake file.  Also, new params must be documented in doc/reST/cli.rst

-- 
Steve Borho