utvideoenc: Enable support for multiple slices and use them

Message ID 1392317958-4268-1-git-send-email-jeebjp@gmail.com
State New
Headers show

Commit Message

Jan Ekström Feb. 13, 2014, 6:59 p.m.
The official Ut Video encoder only threads with slices, thus until
now any files encoded by the libavcodec encoder have only been
decode'able with a single thread. The default slice count is now
set to subsampled_height / 120.

Also sets slices to 1 for the Ut Video encoder tests to keep them
green.
---
 libavcodec/utvideoenc.c | 31 ++++++++++++++++++++++++++++---
 tests/fate/utvideo.mak  |  2 +-
 2 files changed, 29 insertions(+), 4 deletions(-)

Comments

Derek Buitenhuis Feb. 13, 2014, 7:11 p.m. | #1
On 2/13/2014 6:59 PM, Jan Ekström wrote:
> The official Ut Video encoder only threads with slices, thus until
> now any files encoded by the libavcodec encoder have only been
> decode'able with a single thread. The default slice count is now
  ~~~~~~~~~~~
            ^-- decodable


> set to subsampled_height / 120.
> 
> Also sets slices to 1 for the Ut Video encoder tests to keep them
> green.

Add some slice tests too?~

> ---
>  libavcodec/utvideoenc.c | 31 ++++++++++++++++++++++++++++---
>  tests/fate/utvideo.mak  |  2 +-
>  2 files changed, 29 insertions(+), 4 deletions(-)

[...]

> +    /* Check the asked slice count for obviously invalid values (> 256 or negative) */

/*
 * Check the asked slice count for obviously invalid
 * values (> 256 or negative).
 */

> +    if (avctx->slices > 256 || avctx->slices < 0) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "Slice count %d is not supported in Ut Video (theoretical range is 0-256).\n",
> +               avctx->slices);
> +        return AVERROR_INVALIDDATA;
> +    }

Why not make it a warning and use:

av_clip_uint8(avctx->slices)

> +    if (!avctx->slices) {
> +        c->slices = subsampled_height / 120;
> +        if (!c->slices)
> +            c->slices = 1;
> +        else if (c->slices > 256)
> +            c->slices = 256;
> +    } else
> +        c->slices = avctx->slices;

Diego-nit: Braces for consistency

Rest LGTM.

- Derek
Jan Ekström Feb. 13, 2014, 7:30 p.m. | #2
On Thu, Feb 13, 2014 at 9:11 PM, Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
> On 2/13/2014 6:59 PM, Jan Ekström wrote:
>> The official Ut Video encoder only threads with slices, thus until
>> now any files encoded by the libavcodec encoder have only been
>> decode'able with a single thread. The default slice count is now
>   ~~~~~~~~~~~
>             ^-- decodable

Done.

>> Also sets slices to 1 for the Ut Video encoder tests to keep them
>> green.
>
> Add some slice tests too?~

Maybe in the future? I actually tested it against the official decoder
on a subsampled_height = 8 && slices = 8 kind of setup, and I got a
bit-exact result so it seems to work.

>> +    /* Check the asked slice count for obviously invalid values (> 256 or negative) */
>
> /*
>  * Check the asked slice count for obviously invalid
>  * values (> 256 or negative).
>  */

Done.

> Why not make it a warning and use:
>
> av_clip_uint8(avctx->slices)

Because [0,256] is the valid range, not [0,255]?

> Diego-nit: Braces for consistency

Braces added.

Thanks for the review :)


Jan Ekström
Derek Buitenhuis Feb. 13, 2014, 7:40 p.m. | #3
On 2/13/2014 7:30 PM, Jan Ekstrom wrote:
> Because [0,256] is the valid range, not [0,255]?

av_clip then? :)

- Derek
Jan Ekström Feb. 13, 2014, 8:06 p.m. | #4
On Thu, Feb 13, 2014 at 9:40 PM, Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
> On 2/13/2014 7:30 PM, Jan Ekstrom wrote:
>> Because [0,256] is the valid range, not [0,255]?
>
> av_clip then? :)

OK. That's usable. The real issue, though, is that those values would
be invalid, and I do not want to create valid output with invalid
options. Even though a warning is not silent per se, giving a valid
output for invalid input is not something I'd like to do.


Jan Ekström

Patch

diff --git a/libavcodec/utvideoenc.c b/libavcodec/utvideoenc.c
index 47a4f3a..702926a 100644
--- a/libavcodec/utvideoenc.c
+++ b/libavcodec/utvideoenc.c
@@ -58,7 +58,7 @@  static av_cold int utvideo_encode_close(AVCodecContext *avctx)
 static av_cold int utvideo_encode_init(AVCodecContext *avctx)
 {
     UtvideoContext *c = avctx->priv_data;
-    int i;
+    int i, subsampled_height;
     uint32_t original_format;
 
     c->avctx           = avctx;
@@ -132,6 +132,23 @@  static av_cold int utvideo_encode_init(AVCodecContext *avctx)
         return AVERROR_OPTION_NOT_FOUND;
     }
 
+    /* Check the asked slice count for obviously invalid values (> 256 or negative) */
+    if (avctx->slices > 256 || avctx->slices < 0) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Slice count %d is not supported in Ut Video (theoretical range is 0-256).\n",
+               avctx->slices);
+        return AVERROR_INVALIDDATA;
+    }
+
+    /* Check that the slice count is not larger than the subsampled height */
+    subsampled_height = avctx->height >> av_pix_fmt_desc_get(avctx->pix_fmt)->log2_chroma_h;
+    if (avctx->slices > subsampled_height) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Slice count %d is larger than the subsampling-applied height %d.\n",
+               avctx->slices, subsampled_height);
+        return AVERROR_INVALIDDATA;
+    }
+
     avctx->coded_frame = av_frame_alloc();
 
     if (!avctx->coded_frame) {
@@ -181,9 +198,17 @@  static av_cold int utvideo_encode_init(AVCodecContext *avctx)
 
     /*
      * Set how many slices are going to be used.
-     * Set one slice for now.
+     * By default uses multiple slices depending on the subsampled height.
+     * This enables multithreading in the official decoder.
      */
-    c->slices = 1;
+    if (!avctx->slices) {
+        c->slices = subsampled_height / 120;
+        if (!c->slices)
+            c->slices = 1;
+        else if (c->slices > 256)
+            c->slices = 256;
+    } else
+        c->slices = avctx->slices;
 
     /* Set compression mode */
     c->compression = COMP_HUFF;
diff --git a/tests/fate/utvideo.mak b/tests/fate/utvideo.mak
index 161674f..e1ef7ec 100644
--- a/tests/fate/utvideo.mak
+++ b/tests/fate/utvideo.mak
@@ -28,7 +28,7 @@  fate-utvideo_yuv422_median: CMD = framecrc -i $(TARGET_SAMPLES)/utvideo/utvideo_
 FATE_SAMPLES_AVCONV-$(call DEMDEC, AVI, UTVIDEO) += $(FATE_UTVIDEO)
 fate-utvideo: $(FATE_UTVIDEO)
 
-fate-utvideoenc%: CMD = framemd5 -f image2 -vcodec pgmyuv -i $(TARGET_PATH)/tests/vsynth1/%02d.pgm -vcodec utvideo -f avi -sws_flags +accurate_rnd+bitexact ${OPTS}
+fate-utvideoenc%: CMD = framemd5 -f image2 -vcodec pgmyuv -i $(TARGET_PATH)/tests/vsynth1/%02d.pgm -vcodec utvideo -slices 1 -f avi -sws_flags +accurate_rnd+bitexact ${OPTS}
 
 FATE_UTVIDEOENC += fate-utvideoenc_rgba_left
 fate-utvideoenc_rgba_left: OPTS = -pix_fmt rgba -pred left