diff mbox

[FFmpeg-devel] speedhq: fix behavior of single-field decoding

Message ID E1dcfyM-0001yn-UY@pannekake.samfundet.no
State Superseded
Headers show

Commit Message

Steinar H. Gunderson Aug. 1, 2017, 10:48 p.m. UTC 
The height convention for decoding frames with only a single field made sense
for compatibility with legacy decoders, but doesn't really match the convention
used by NDI, which is the primary (only?) user. Thus, change it to simply
assuming that if the two fields overlap, the frame is meant to be a single
field and the frame height matches the field height.

Also add simple FATE tests, based on output produced by the NDI SDK.

Add myself as speedhq maintainer, per request.
---
 MAINTAINERS                            |  2 ++
 libavcodec/speedhq.c                   | 15 +++++++++------
 tests/Makefile                         |  1 +
 tests/fate/speedhq.mak                 |  8 ++++++++
 tests/fate/vcodec.mak                  |  2 ++
 tests/ref/fate/speedhq-422             |  6 ++++++
 tests/ref/fate/speedhq-422-singlefield |  6 ++++++
 7 files changed, 34 insertions(+), 6 deletions(-)
 create mode 100644 tests/fate/speedhq.mak
 create mode 100644 tests/ref/fate/speedhq-422
 create mode 100644 tests/ref/fate/speedhq-422-singlefield

Comments

Steinar H. Gunderson Aug. 1, 2017, 10:51 p.m. UTC | #1 
On Wed, Aug 02, 2017 at 12:48:38AM +0200, Steinar H. Gunderson wrote:
> Also add simple FATE tests, based on output produced by the NDI SDK.

Here are the required samples. I couldn't find much documentation on how to
add tests to FATE, so everything has been done by cargo culting.

/* Steinar */
Michael Niedermayer Aug. 2, 2017, 1:18 a.m. UTC | #2 
On Wed, Aug 02, 2017 at 12:48:38AM +0200, Steinar H. Gunderson wrote:
> The height convention for decoding frames with only a single field made sense
> for compatibility with legacy decoders, but doesn't really match the convention
> used by NDI, which is the primary (only?) user. Thus, change it to simply
> assuming that if the two fields overlap, the frame is meant to be a single
> field and the frame height matches the field height.
> 

> Also add simple FATE tests, based on output produced by the NDI SDK.

This seems to break a full "make fate"

reference file './tests/ref/vsynth/vsynth1-speedhq' not found
Test vsynth1-speedhq failed. Look at tests/data/fate/vsynth1-speedhq.err for details.

adding ./tests/ref/vsynth/vsynth1-speedhq results the in:
make: *** [fate-vsynth1-speedhq] Error 1

the error is
Unknown encoder 'speedhq'


[...]
Michael Niedermayer Aug. 2, 2017, 1:26 a.m. UTC | #3 
On Wed, Aug 02, 2017 at 12:51:17AM +0200, Steinar H. Gunderson wrote:
> On Wed, Aug 02, 2017 at 12:48:38AM +0200, Steinar H. Gunderson wrote:
> > Also add simple FATE tests, based on output produced by the NDI SDK.
> 
> Here are the required samples. I couldn't find much documentation on how to
> add tests to FATE, so everything has been done by cargo culting.

files uploaded

thx

[...]
James Almer Aug. 2, 2017, 1:44 a.m. UTC | #4 
On 8/1/2017 7:48 PM, Steinar H. Gunderson wrote:
> The height convention for decoding frames with only a single field made sense
> for compatibility with legacy decoders, but doesn't really match the convention
> used by NDI, which is the primary (only?) user. Thus, change it to simply
> assuming that if the two fields overlap, the frame is meant to be a single
> field and the frame height matches the field height.
> 
> Also add simple FATE tests, based on output produced by the NDI SDK.
> 
> Add myself as speedhq maintainer, per request.

This should ideally be split in two or three patches, one per
addition/change.

> ---
>  MAINTAINERS                            |  2 ++
>  libavcodec/speedhq.c                   | 15 +++++++++------
>  tests/Makefile                         |  1 +
>  tests/fate/speedhq.mak                 |  8 ++++++++
>  tests/fate/vcodec.mak                  |  2 ++
>  tests/ref/fate/speedhq-422             |  6 ++++++
>  tests/ref/fate/speedhq-422-singlefield |  6 ++++++
>  7 files changed, 34 insertions(+), 6 deletions(-)
>  create mode 100644 tests/fate/speedhq.mak
>  create mode 100644 tests/ref/fate/speedhq-422
>  create mode 100644 tests/ref/fate/speedhq-422-singlefield
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae0e08d121..ce5e1dae08 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -233,6 +233,7 @@ Codecs:
>    smvjpegdec.c                          Ash Hughes
>    snow*                                 Michael Niedermayer, Loren Merritt
>    sonic.c                               Alex Beregszaszi
> +  speedhq.c                             Steinar H. Gunderson
>    srt*                                  Aurelien Jacobs
>    sunrast.c                             Ivo van Poorten
>    svq3.c                                Michael Niedermayer
> @@ -598,6 +599,7 @@ Reynaldo H. Verdejo Pinochet  6E27 CD34 170C C78E 4D4F 5F40 C18E 077F 3114 452A
>  Robert Swain                  EE7A 56EA 4A81 A7B5 2001 A521 67FA 362D A2FC 3E71
>  Sascha Sommer                 38A0 F88B 868E 9D3A 97D4 D6A0 E823 706F 1E07 0D3C
>  Stefano Sabatini              0D0B AD6B 5330 BBAD D3D6 6A0C 719C 2839 FC43 2D5F
> +Steinar H. Gunderson          C2E9 004F F028 C18E 4EAD DB83 7F61 7561 7797 8F76
>  Stephan Hilb                  4F38 0B3A 5F39 B99B F505 E562 8D5C 5554 4E17 8863
>  Tiancheng "Timothy" Gu        9456 AFC0 814A 8139 E994 8351 7FE6 B095 B582 B0D4
>  Tim Nicholson                 38CF DB09 3ED0 F607 8B67 6CED 0C0B FC44 8B0B FC83
> diff --git a/libavcodec/speedhq.c b/libavcodec/speedhq.c
> index 60efb0222b..eca45beb67 100644
> --- a/libavcodec/speedhq.c
> +++ b/libavcodec/speedhq.c
> @@ -448,12 +448,15 @@ static int speedhq_decode_frame(AVCodecContext *avctx,
>      frame->key_frame = 1;
>  
>      if (second_field_offset == 4) {
> -        /*
> -         * Overlapping first and second fields is used to signal
> -         * encoding only a single field (the second field then comes
> -         * as a separate, later frame).
> -         */
> -        frame->height >>= 1;
> +	/*
> +	 * Overlapping first and second fields is used to signal
> +	 * encoding only a single field. In this case, "height"
> +	 * is ambiguous; it could mean either the height of the
> +	 * frame as a whole, or of the field. The former would make
> +	 * more sense for compatibility with legacy decoders,
> +	 * but this matches the convention used in NDI, which is
> +	 * the primary user of this trick.
> +	 */
>          if ((ret = decode_speedhq_field(s, buf, buf_size, frame, 0, 4, buf_size, 1)) < 0)
>              return ret;
>      } else {
> diff --git a/tests/Makefile b/tests/Makefile
> index ab83ae855d..f9b9cf4188 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -164,6 +164,7 @@ include $(SRC_PATH)/tests/fate/qt.mak
>  include $(SRC_PATH)/tests/fate/qtrle.mak
>  include $(SRC_PATH)/tests/fate/real.mak
>  include $(SRC_PATH)/tests/fate/screen.mak
> +include $(SRC_PATH)/tests/fate/speedhq.mak
>  include $(SRC_PATH)/tests/fate/source.mak
>  include $(SRC_PATH)/tests/fate/subtitles.mak
>  include $(SRC_PATH)/tests/fate/utvideo.mak
> diff --git a/tests/fate/speedhq.mak b/tests/fate/speedhq.mak
> new file mode 100644
> index 0000000000..a5c2fb99a9
> --- /dev/null
> +++ b/tests/fate/speedhq.mak
> @@ -0,0 +1,8 @@
> +FATE_SPEEDHQ = fate-speedhq-422                                           \
> +               fate-speedhq-422-singlefield                               \
> +
> +FATE_SAMPLES_AVCONV-$(call ALLYES, SPEEDHQ_DECODER) += $(FATE_SPEEDHQ)

This depends also on the rawvideo demuxer, so it needs call DEMDEC.
Also, use FATE_SAMPLES_FFMPEG, since it's a test added in our tree and
not elsewhere.

FATE_SAMPLES_FFMPEG-$(call DEMDEC, RAWVIDEO, SPEEDHQ) += $(FATE_SPEEDHQ)

> +fate-speedhq: $(FATE_SPEEDHQ)
> +
> +fate-speedhq-422:             CMD = framecrc -flags +bitexact -f rawvideo -codec speedhq -vtag SHQ2 -video_size 112x64 -i $(TARGET_SAMPLES)/speedhq/progressive.shq2 -pix_fmt yuv422p
> +fate-speedhq-422-singlefield: CMD = framecrc -flags +bitexact -f rawvideo -codec speedhq -vtag SHQ2 -video_size 112x32 -i $(TARGET_SAMPLES)/speedhq/singlefield.shq2 -pix_fmt yuv422p

Use -c:v and -tag:v, not -codec and -vtag.

> diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak
> index 8c24510937..0a284ecbb9 100644
> --- a/tests/fate/vcodec.mak
> +++ b/tests/fate/vcodec.mak
> @@ -386,6 +386,8 @@ fate-vsynth%-snow-hpel:          ENCOPTS = -qscale 2              \
>  fate-vsynth%-snow-ll:            ENCOPTS = -qscale .001 -pred 1 \
>                                             -flags +mv4+qpel
>  
> +FATE_VCODEC-$(call ALLYES, SPEEDHQ_DECODER) += speedhq

This is wrong. You're trying to add a test meant for encoders.

> +
>  FATE_VCODEC-$(call ENCDEC, SVQ1, MOV)   += svq1
>  fate-vsynth%-svq1:               ENCOPTS = -qscale 3 -pix_fmt yuv410p
>  fate-vsynth%-svq1:               FMT     = mov
> diff --git a/tests/ref/fate/speedhq-422 b/tests/ref/fate/speedhq-422
> new file mode 100644
> index 0000000000..7bb0d2388d
> --- /dev/null
> +++ b/tests/ref/fate/speedhq-422
> @@ -0,0 +1,6 @@
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: rawvideo
> +#dimensions 0: 112x64
> +#sar 0: 0/1
> +0,          0,          0,        1,    14336, 0x9bb6dc6d
> diff --git a/tests/ref/fate/speedhq-422-singlefield b/tests/ref/fate/speedhq-422-singlefield
> new file mode 100644
> index 0000000000..343c52645c
> --- /dev/null
> +++ b/tests/ref/fate/speedhq-422-singlefield
> @@ -0,0 +1,6 @@
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: rawvideo
> +#dimensions 0: 112x32
> +#sar 0: 0/1
> +0,          0,          0,        1,     7168, 0x75de4109
>
Steinar H. Gunderson Aug. 2, 2017, 7:43 a.m. UTC | #5 
On Wed, Aug 02, 2017 at 03:18:49AM +0200, Michael Niedermayer wrote:
> This seems to break a full "make fate"

OK, removing the offending line and resending.

/* Steinar */
diff mbox

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index ae0e08d121..ce5e1dae08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -233,6 +233,7 @@  Codecs:
   smvjpegdec.c                          Ash Hughes
   snow*                                 Michael Niedermayer, Loren Merritt
   sonic.c                               Alex Beregszaszi
+  speedhq.c                             Steinar H. Gunderson
   srt*                                  Aurelien Jacobs
   sunrast.c                             Ivo van Poorten
   svq3.c                                Michael Niedermayer
@@ -598,6 +599,7 @@  Reynaldo H. Verdejo Pinochet  6E27 CD34 170C C78E 4D4F 5F40 C18E 077F 3114 452A
 Robert Swain                  EE7A 56EA 4A81 A7B5 2001 A521 67FA 362D A2FC 3E71
 Sascha Sommer                 38A0 F88B 868E 9D3A 97D4 D6A0 E823 706F 1E07 0D3C
 Stefano Sabatini              0D0B AD6B 5330 BBAD D3D6 6A0C 719C 2839 FC43 2D5F
+Steinar H. Gunderson          C2E9 004F F028 C18E 4EAD DB83 7F61 7561 7797 8F76
 Stephan Hilb                  4F38 0B3A 5F39 B99B F505 E562 8D5C 5554 4E17 8863
 Tiancheng "Timothy" Gu        9456 AFC0 814A 8139 E994 8351 7FE6 B095 B582 B0D4
 Tim Nicholson                 38CF DB09 3ED0 F607 8B67 6CED 0C0B FC44 8B0B FC83
diff --git a/libavcodec/speedhq.c b/libavcodec/speedhq.c
index 60efb0222b..eca45beb67 100644
--- a/libavcodec/speedhq.c
+++ b/libavcodec/speedhq.c
@@ -448,12 +448,15 @@  static int speedhq_decode_frame(AVCodecContext *avctx,
     frame->key_frame = 1;
 
     if (second_field_offset == 4) {
-        /*
-         * Overlapping first and second fields is used to signal
-         * encoding only a single field (the second field then comes
-         * as a separate, later frame).
-         */
-        frame->height >>= 1;
+	/*
+	 * Overlapping first and second fields is used to signal
+	 * encoding only a single field. In this case, "height"
+	 * is ambiguous; it could mean either the height of the
+	 * frame as a whole, or of the field. The former would make
+	 * more sense for compatibility with legacy decoders,
+	 * but this matches the convention used in NDI, which is
+	 * the primary user of this trick.
+	 */
         if ((ret = decode_speedhq_field(s, buf, buf_size, frame, 0, 4, buf_size, 1)) < 0)
             return ret;
     } else {
diff --git a/tests/Makefile b/tests/Makefile
index ab83ae855d..f9b9cf4188 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -164,6 +164,7 @@  include $(SRC_PATH)/tests/fate/qt.mak
 include $(SRC_PATH)/tests/fate/qtrle.mak
 include $(SRC_PATH)/tests/fate/real.mak
 include $(SRC_PATH)/tests/fate/screen.mak
+include $(SRC_PATH)/tests/fate/speedhq.mak
 include $(SRC_PATH)/tests/fate/source.mak
 include $(SRC_PATH)/tests/fate/subtitles.mak
 include $(SRC_PATH)/tests/fate/utvideo.mak
diff --git a/tests/fate/speedhq.mak b/tests/fate/speedhq.mak
new file mode 100644
index 0000000000..a5c2fb99a9
--- /dev/null
+++ b/tests/fate/speedhq.mak
@@ -0,0 +1,8 @@ 
+FATE_SPEEDHQ = fate-speedhq-422                                           \
+               fate-speedhq-422-singlefield                               \
+
+FATE_SAMPLES_AVCONV-$(call ALLYES, SPEEDHQ_DECODER) += $(FATE_SPEEDHQ)
+fate-speedhq: $(FATE_SPEEDHQ)
+
+fate-speedhq-422:             CMD = framecrc -flags +bitexact -f rawvideo -codec speedhq -vtag SHQ2 -video_size 112x64 -i $(TARGET_SAMPLES)/speedhq/progressive.shq2 -pix_fmt yuv422p
+fate-speedhq-422-singlefield: CMD = framecrc -flags +bitexact -f rawvideo -codec speedhq -vtag SHQ2 -video_size 112x32 -i $(TARGET_SAMPLES)/speedhq/singlefield.shq2 -pix_fmt yuv422p
diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak
index 8c24510937..0a284ecbb9 100644
--- a/tests/fate/vcodec.mak
+++ b/tests/fate/vcodec.mak
@@ -386,6 +386,8 @@  fate-vsynth%-snow-hpel:          ENCOPTS = -qscale 2              \
 fate-vsynth%-snow-ll:            ENCOPTS = -qscale .001 -pred 1 \
                                            -flags +mv4+qpel
 
+FATE_VCODEC-$(call ALLYES, SPEEDHQ_DECODER) += speedhq
+
 FATE_VCODEC-$(call ENCDEC, SVQ1, MOV)   += svq1
 fate-vsynth%-svq1:               ENCOPTS = -qscale 3 -pix_fmt yuv410p
 fate-vsynth%-svq1:               FMT     = mov
diff --git a/tests/ref/fate/speedhq-422 b/tests/ref/fate/speedhq-422
new file mode 100644
index 0000000000..7bb0d2388d
--- /dev/null
+++ b/tests/ref/fate/speedhq-422
@@ -0,0 +1,6 @@ 
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 112x64
+#sar 0: 0/1
+0,          0,          0,        1,    14336, 0x9bb6dc6d
diff --git a/tests/ref/fate/speedhq-422-singlefield b/tests/ref/fate/speedhq-422-singlefield
new file mode 100644
index 0000000000..343c52645c
--- /dev/null
+++ b/tests/ref/fate/speedhq-422-singlefield
@@ -0,0 +1,6 @@ 
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 112x32
+#sar 0: 0/1
+0,          0,          0,        1,     7168, 0x75de4109