[FFmpeg-devel,v3,2/3] avcodec/bitpacked: add interlace support

From: Damien Riegel <damien.riegel@savoirfairelinux.com>

This codec is already capable of depacking some combinations of pixel
formats and depth as defined in the RFC4175. The only difference between
progressive and interlace is that either a packet will contain the whole
frame, or only a field of the frame.

There is no mechanism for interlaced frames reconstruction at the rtp
demux level, so it has to be handled by the codec which needs to
partially recompose an AVFrame with every incoming field AVPacket.
A frame is ouput only when the frame is completed with the 2nd field
(bottom).

The additionnal field flags in AVPacket allow the decoder to dynamically
determine the frame format, i.e. progressive or interlaced.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
---

Change v2 -> v3:
 Let the decoder determine if the stream is interlaced or progressive
 by fixing top field flag and adding bottom fields flag.
 Thanks Rostislav for bringing us some context.

---
 libavcodec/avcodec.h   |   8 ++++
 libavcodec/bitpacked.c | 112 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 106 insertions(+), 14 deletions(-)

On 24 April 2018 at 19:54, Patrick Keroulas <
patrick.keroulas@savoirfairelinux.com> wrote:

> From: Damien Riegel <damien.riegel@savoirfairelinux.com>
>
> This codec is already capable of depacking some combinations of pixel
> formats and depth as defined in the RFC4175. The only difference between
> progressive and interlace is that either a packet will contain the whole
> frame, or only a field of the frame.
>
> There is no mechanism for interlaced frames reconstruction at the rtp
> demux level, so it has to be handled by the codec which needs to
> partially recompose an AVFrame with every incoming field AVPacket.
> A frame is ouput only when the frame is completed with the 2nd field
> (bottom).
>
> The additionnal field flags in AVPacket allow the decoder to dynamically
> determine the frame format, i.e. progressive or interlaced.
>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> ---
>
> Change v2 -> v3:
>  Let the decoder determine if the stream is interlaced or progressive
>  by fixing top field flag and adding bottom fields flag.
>  Thanks Rostislav for bringing us some context.
>
> ---
>  libavcodec/avcodec.h   |   8 ++++
>  libavcodec/bitpacked.c | 112 ++++++++++++++++++++++++++++++
> ++++++++++++-------
>  2 files changed, 106 insertions(+), 14 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index fb0c6fa..14811be 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
>   */
>  #define AV_PKT_FLAG_DISPOSABLE 0x0010
>
> +/**
> + * The packet contains a top field.
> + */
> +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> +/**
> + * The packet contains a bottom field.
> + */
> +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
>
>  enum AVSideDataParamChangeFlags {
>      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> diff --git a/libavcodec/bitpacked.c b/libavcodec/bitpacked.c
> index 85d4bdd..d0237c6 100644
> --- a/libavcodec/bitpacked.c
> +++ b/libavcodec/bitpacked.c
> @@ -32,8 +32,9 @@
>  #include "libavutil/imgutils.h"
>
>  struct BitpackedContext {
> -    int (*decode)(AVCodecContext *avctx, AVFrame *frame,
> -                  AVPacket *pkt);
> +    int (*decode)(AVCodecContext *avctx, AVFrame *frame, AVPacket *pkt);
> +    AVFrame *cur_interlaced_frame;
> +    int prev_top_field;
>  };
>
>  /* For this format, it's a simple passthrough */
> @@ -42,6 +43,9 @@ static int bitpacked_decode_uyvy422(AVCodecContext
> *avctx, AVFrame *frame,
>  {
>      int ret;
>
> +    if (frame->interlaced_frame)
> +        return AVERROR_PATCHWELCOME;
>

No need to check it here, you already reject it on init.



> +
>      /* there is no need to copy as the data already match
>       * a known pixel format */
>      frame->buf[0] = av_buffer_ref(avpkt->buf);
> @@ -60,13 +64,19 @@ static int bitpacked_decode_yuv422p10(AVCodecContext
> *avctx, AVFrame *frame,
>  {
>      uint64_t frame_size = (uint64_t)avctx->width *
> (uint64_t)avctx->height * 20;
>      uint64_t packet_size = (uint64_t)avpkt->size * 8;
> +    int interlaced = frame->interlaced_frame;
> +    int top_field = (avpkt->flags & AV_PKT_FLAG_TOP_FIELD) ? 1 : 0;
>      GetBitContext bc;
>      uint16_t *y, *u, *v;
>      int ret, i, j;
>
> -
> -    if (frame_size > packet_size)
> +    /* check packet size depending on the interlaced/progressive format */
> +    if (interlaced) {
> +        if ((frame_size >> 1) > packet_size)
> +            return AVERROR_INVALIDDATA;
> +    } else if (frame_size > packet_size) {
>          return AVERROR_INVALIDDATA;
> +    }
>
>      if (avctx->width % 2)
>          return AVERROR_PATCHWELCOME;
> @@ -75,7 +85,18 @@ static int bitpacked_decode_yuv422p10(AVCodecContext
> *avctx, AVFrame *frame,
>      if (ret)
>          return ret;
>
> -    for (i = 0; i < avctx->height; i++) {
> +    /*
> +     * if the frame is interlaced, the avpkt we are getting is either the
> top
> +     * or the bottom field. If it's the bottom field, it contains all the
> odd
> +     * lines of the recomposed frame, so we start at offset 1.
> +     */
> +    i = (interlaced && !top_field) ? 1 : 0;
>

Could you move this above the init_get_bits call? That will make it clearer
where the existing asm function which is more than 4 times faster would go
to. Doubt you care about speed but some people do.


+
> +    /*
> +     * Packets from interlaced frames contain either even lines, or odd
> +     * lines, so increment by two in that case.
> +     */
> +    for (; i < avctx->height; interlaced ? i += 2 : i++) {
>

No need to use ternary here, just do:
for (; i < avctx->height; i+= 1 + interlaced) {


         y = (uint16_t*)(frame->data[0] + i * frame->linesize[0]);
>          u = (uint16_t*)(frame->data[1] + i * frame->linesize[1]);
>          v = (uint16_t*)(frame->data[2] + i * frame->linesize[2]);
> @@ -100,17 +121,35 @@ static av_cold int bitpacked_init_decoder(AVCodecContext
> *avctx)
>
>      if (avctx->codec_tag == MKTAG('U', 'Y', 'V', 'Y')) {
>          if (avctx->bits_per_coded_sample == 16 &&
> -            avctx->pix_fmt == AV_PIX_FMT_UYVY422)
> +            avctx->pix_fmt == AV_PIX_FMT_UYVY422) {
> +
> +            if (avctx->field_order > AV_FIELD_PROGRESSIVE) {
> +                av_log(avctx, AV_LOG_ERROR, "interlaced not yet supported
> for 8-bit\n");
> +                return AVERROR_PATCHWELCOME;
> +            }
> +
>              bc->decode = bitpacked_decode_uyvy422;
> -        else if (avctx->bits_per_coded_sample == 20 &&
> -                 avctx->pix_fmt == AV_PIX_FMT_YUV422P10)
> +        } else if (avctx->bits_per_coded_sample == 20 &&
> +                 avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
>              bc->decode = bitpacked_decode_yuv422p10;
> -        else
> +        } else {
>              return AVERROR_INVALIDDATA;
> +        }
>      } else {
>          return AVERROR_INVALIDDATA;
>      }
>
> +    bc->cur_interlaced_frame = av_frame_alloc();
> +
> +    return 0;
> +}
> +
> +static av_cold int bitpacked_end_decoder(AVCodecContext *avctx)
> +{
> +    struct BitpackedContext *bc = avctx->priv_data;
> +
> +    av_frame_free(&bc->cur_interlaced_frame);
> +
>      return 0;
>  }
>
> @@ -131,13 +170,57 @@ static int bitpacked_decode(AVCodecContext *avctx,
> void *data, int *got_frame,
>              return res;
>      }
>
> -    res = bc->decode(avctx, frame, avpkt);
> -    if (res)
> -        return res;
> +    if (avpkt->flags & AV_PKT_FLAG_TOP_FIELD) {
> +        if (bc->prev_top_field)
> +            av_log(avctx, AV_LOG_WARNING, "Consecutive top field
> detected.\n");
>
> -    *got_frame = 1;
> -    return buf_size;
> +        frame->interlaced_frame = 1;
> +        frame->top_field_first = 1;
>
> +        /* always decode the top (1st) field and ref the result frame
> +         * but don't output anything */
> +        if ((res = bc->decode(avctx, frame, avpkt)) < 0)
> +            return res;
> +
> +        av_frame_unref(bc->cur_interlaced_frame);
> +        if ((res = av_frame_ref(bc->cur_interlaced_frame, frame)) < 0)
> +            return res;
> +
> +        bc->prev_top_field = 1;
> +
> +        return 0;
> +    } else if (avpkt->flags & AV_PKT_FLAG_BOTTOM_FIELD) {
> +        if (!bc->prev_top_field) {
> +            av_log(avctx, AV_LOG_WARNING, "Drop consecutive bottom
> field.\n");
>

"Top field missing!\n", sounds better.


+            return 0;
> +        }
> +
> +        frame->interlaced_frame = 1;
> +        frame->top_field_first = 1;
> +
> +        /* complete the ref'd frame with bottom field and output the
> +         * result */
> +        if ((res = bc->decode(avctx, bc->cur_interlaced_frame, avpkt)) <
> 0)
> +            return res;
> +
> +        av_frame_unref(frame);
>

You don't need to unref here, the input frame is empty.


+        if ((res = av_frame_ref(frame, bc->cur_interlaced_frame)) < 0)
> +            return res;
> +
> +        bc->prev_top_field = 0;
> +        *got_frame = 1;
> +        return buf_size;
> +    } else if (!(avpkt->flags & (AV_PKT_FLAG_TOP_FIELD |
> AV_PKT_FLAG_BOTTOM_FIELD))) {
> +        /* No field: the frame is progressive */
> +        if ((res = bc->decode(avctx, frame, avpkt)) < 0)
> +            return res;
> +
>

You need to drop the top field here if it exists.


+        *got_frame = 1;
> +        return buf_size;
> +    } else {
> +        av_log(avctx, AV_LOG_WARNING, "Wrong field flags.\n");
>

This won't ever get called, remove it. You already check for bottom, top or
neither.