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

James Almer jamrial at gmail.com
Wed Aug 2 04:44:58 EEST 2017


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
> 



More information about the ffmpeg-devel mailing list