[FFmpeg-devel] [PATCH v10 1/2] lavc/svt_hevc: add libsvt hevc encoder wrapper

[Sun, Jing A]

On Fri, Mar 29, 2019 at 5:28 AM Jing Sun <jing.a.sun at intel.com> wrote:
+static int config_enc_params(EB_H265_ENC_CONFIGURATION *param,
+                             AVCodecContext *avctx)
+{
+    SvtContext *svt_enc = avctx->priv_data;
+    int ret;
+
+    param->sourceWidth = avctx->width;
+    param->sourceHeight = avctx->height;
+    param->encoderBitDepth = 8;
+
+    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P10LE) {
+        av_log(avctx, AV_LOG_DEBUG, "Encoder 10 bits depth input\n");
+
+        param->encoderBitDepth = 10;
+    }
+    param->encoderColorFormat = EB_YUV420;

>this patch blatantly ignores my review(s) and therefore it is rejected

>to reiterate:
>- if the encoder does not support 10 bit (or if scales down from 10 to 8 internally), this feature should not be present in the wrapper either (or it should at least warn the user)

>- if the encoder does not support setting the color properties in the VUI, the wrapper should definitely warn the user of the loss of information (or we should wait until this features is present upstream)

>reagrds
>-- 
>Vittorio

----------------------------------
So what is happening in this case? The encoder is slower because it converts from 10 to 8 bit internally? And then the output encode is 8 bit right now? If that is the case, I'd rather have this functionality removed since the conversion can happen directly within ffmpeg. Having the conversion performed by the encoder is guaranteed to be slower and less precise, and if the output is not 10 bit it is very surprising too.

At the same time the comment in the code is useless because users will never read something buried deep in the code, I'd suggest printing something at the warning level so that it will be shown during the conversion (and please have it proofread by a native English-speaking person).
---------------------------------

You said that functionality should be removed and the comment in the code is useless, so I removed it. The 10 bit works fine this way, no need to warn the users.


----------------------------------
nit: excessive whitespace alignment
---------------------------------- 
I have corrected the whitespace.


----------------------------------
Where is the warning that notifies the lack of color properties support?
----------------------------------
I was with you, so added the warning in v9, then I removed the hdr/vui interfaces entirely in v10, after I saw mark's review. I think you are right, that the SVT-HEVC's implementation of hdr/vui is not a complete one , hence the removal. And I have explained the removal in the former reply: to add hdr back once the feature is fully supported in SVT-HEVC.


----------------------------------
IMO these options help text could be improved. 
----------------------------------
I have improved it in v9/v10. 
+    { "aud", "Include Access Unit Delimiter", OFFSET(aud),
+      AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },

- Jing