[FFmpeg-devel,v5,1/2] avcodec/bitpacked: add interlace support

Submitted by Patrick Keroulas on May 14, 2018, 7:29 p.m.

Details

Message ID 1526326150-12360-1-git-send-email-patrick.keroulas@savoirfairelinux.com
State New
Headers show

Commit Message

Patrick Keroulas May 14, 2018, 7:29 p.m.
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: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---

Change v4 -> v5:
 Call ff_get_buffer only when necessary.
---
 libavcodec/avcodec.h   |   8 ++++
 libavcodec/bitpacked.c | 127 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 118 insertions(+), 17 deletions(-)

Comments

Rostislav Pehlivanov May 14, 2018, 8:20 p.m.
On 14 May 2018 at 20:29, 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: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>
> Change v4 -> v5:
>  Call ff_get_buffer only when necessary.
> ---
>  libavcodec/avcodec.h   |   8 ++++
>  libavcodec/bitpacked.c | 127 ++++++++++++++++++++++++++++++
> ++++++++++++-------
>  2 files changed, 118 insertions(+), 17 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
>

Add a doc/APIchanges entry and bump lavc minor version.
Also make it a separate patch.



>  enum AVSideDataParamChangeFlags {
>      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> diff --git a/libavcodec/bitpacked.c b/libavcodec/bitpacked.c
> index f0b417d..7c5221d 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 */
> @@ -60,25 +61,39 @@ static int bitpacked_decode_yuv422p10(AVCodecContext
> *avctx, AVFrame *frame,
>  {
>      uint64_t frame_size = (uint64_t)avctx->width *
> (uint64_t)avctx->height * 20;
>

20LL will promote the multiplication to 64 bits without needing to cast
both the width and the height.



>      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;
>
> -    ret = ff_get_buffer(avctx, frame, 0);
> -    if (ret < 0)
> -        return ret;
> -
> -    if (frame_size > packet_size)
> +    /* check packet size depending on the interlaced/progressive format */
> +    if (interlaced) {
> +        if ((frame_size / 2) > packet_size)
> +            return AVERROR_INVALIDDATA;
> +    } else if (frame_size > packet_size) {
>          return AVERROR_INVALIDDATA;
> +    }
>
>      if (avctx->width % 2)
>          return AVERROR_PATCHWELCOME;
>
> +    /*
> +     * 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;
> +

     ret = init_get_bits(&bc, avpkt->data, avctx->width * avctx->height *
> 20);
>

You already calculate width*height*20, in frame_size, reuse it.


     if (ret)
>          return ret;
>
> -    for (i = 0; i < avctx->height; i++) {
> +    /*
> +     * Packets from interlaced frames contain either even lines, or odd
> +     * lines, so increment by two in that case.
> +     */
> +    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]);
> @@ -103,17 +118,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;
>  }
>
> @@ -128,13 +161,72 @@ static int bitpacked_decode(AVCodecContext *avctx,
> void *data, int *got_frame,
>      frame->pict_type = AV_PICTURE_TYPE_I;
>      frame->key_frame = 1;
>
> -    res = bc->decode(avctx, frame, avpkt);
> -    if (res)
> -        return res;
> +    if ((avpkt->flags & AV_PKT_FLAG_TOP_FIELD) &&
> +        (avpkt->flags & AV_PKT_FLAG_BOTTOM_FIELD)) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid field flags.\n");
> +        return AVERROR_INVALIDDATA;
> +    } else if (avpkt->flags & AV_PKT_FLAG_TOP_FIELD) {
> +        if (bc->prev_top_field)
> +            av_log(avctx, AV_LOG_WARNING, "Consecutive top field
> detected.\n");
>

Drop this warning, its unneeded and will be printed if a switch or a seek
happens.


+
> +        frame->interlaced_frame = 1;
> +        frame->top_field_first = 1;
> +
> +        if (avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
>

Why do you check the pixfmt? This makes no sense, here or at all, its
already guaranteed to be a supported pixfmt.


+            res = ff_get_buffer(avctx, frame, 0);
> +            if (res < 0)
> +                return res;
> +        }
> +
> +        /* 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, "Top field missing.\n");
> +            return 0;
>

Make it an AV_LOG_ERROR, this is more serious than a warning.
also return AVERROR_INVALIDDATA;



> +        }
> +
> +        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;
> +
> +        if ((res = av_frame_ref(frame, bc->cur_interlaced_frame)) < 0)
> +            return res;
>
> -    *got_frame = 1;
> -    return buf_size;
> +        bc->prev_top_field = 0;
> +        *got_frame = 1;
> +        return buf_size;
> +    } else {
> +        /* No field: the frame is progressive. */
> +        if (bc->prev_top_field)
> +            av_frame_unref(bc->cur_interlaced_frame);
> +
> +        if (avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
>

Same, there's no need for a pixfmt check. This also breaks UYVY. Why did
you put the check?


+            res = ff_get_buffer(avctx, frame, 0);
> +            if (res < 0)
> +                return res;
> +        }
>
> +        if ((res = bc->decode(avctx, frame, avpkt)) < 0)
> +            return res;
> +
> +        *got_frame = 1;
> +        return buf_size;
> +    }
>  }
>
>  AVCodec ff_bitpacked_decoder = {
> @@ -144,6 +236,7 @@ AVCodec ff_bitpacked_decoder = {
>      .id = AV_CODEC_ID_BITPACKED,
>      .priv_data_size        = sizeof(struct BitpackedContext),
>      .init = bitpacked_init_decoder,
> +    .close = bitpacked_end_decoder,
>      .decode = bitpacked_decode,
>      .capabilities = AV_CODEC_CAP_EXPERIMENTAL,
>  };
> --
> 2.7.4
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Overall, if you fix those it should be pretty much ready.

Patch hide | download patch | download mbox

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 f0b417d..7c5221d 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 */
@@ -60,25 +61,39 @@  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;
 
-    ret = ff_get_buffer(avctx, frame, 0);
-    if (ret < 0)
-        return ret;
-
-    if (frame_size > packet_size)
+    /* check packet size depending on the interlaced/progressive format */
+    if (interlaced) {
+        if ((frame_size / 2) > packet_size)
+            return AVERROR_INVALIDDATA;
+    } else if (frame_size > packet_size) {
         return AVERROR_INVALIDDATA;
+    }
 
     if (avctx->width % 2)
         return AVERROR_PATCHWELCOME;
 
+    /*
+     * 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;
+
     ret = init_get_bits(&bc, avpkt->data, avctx->width * avctx->height * 20);
     if (ret)
         return ret;
 
-    for (i = 0; i < avctx->height; i++) {
+    /*
+     * Packets from interlaced frames contain either even lines, or odd
+     * lines, so increment by two in that case.
+     */
+    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]);
@@ -103,17 +118,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;
 }
 
@@ -128,13 +161,72 @@  static int bitpacked_decode(AVCodecContext *avctx, void *data, int *got_frame,
     frame->pict_type = AV_PICTURE_TYPE_I;
     frame->key_frame = 1;
 
-    res = bc->decode(avctx, frame, avpkt);
-    if (res)
-        return res;
+    if ((avpkt->flags & AV_PKT_FLAG_TOP_FIELD) &&
+        (avpkt->flags & AV_PKT_FLAG_BOTTOM_FIELD)) {
+        av_log(avctx, AV_LOG_WARNING, "Invalid field flags.\n");
+        return AVERROR_INVALIDDATA;
+    } else if (avpkt->flags & AV_PKT_FLAG_TOP_FIELD) {
+        if (bc->prev_top_field)
+            av_log(avctx, AV_LOG_WARNING, "Consecutive top field detected.\n");
+
+        frame->interlaced_frame = 1;
+        frame->top_field_first = 1;
+
+        if (avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
+            res = ff_get_buffer(avctx, frame, 0);
+            if (res < 0)
+                return res;
+        }
+
+        /* 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, "Top field missing.\n");
+            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;
+
+        if ((res = av_frame_ref(frame, bc->cur_interlaced_frame)) < 0)
+            return res;
 
-    *got_frame = 1;
-    return buf_size;
+        bc->prev_top_field = 0;
+        *got_frame = 1;
+        return buf_size;
+    } else {
+        /* No field: the frame is progressive. */
+        if (bc->prev_top_field)
+            av_frame_unref(bc->cur_interlaced_frame);
+
+        if (avctx->pix_fmt == AV_PIX_FMT_YUV422P10) {
+            res = ff_get_buffer(avctx, frame, 0);
+            if (res < 0)
+                return res;
+        }
 
+        if ((res = bc->decode(avctx, frame, avpkt)) < 0)
+            return res;
+
+        *got_frame = 1;
+        return buf_size;
+    }
 }
 
 AVCodec ff_bitpacked_decoder = {
@@ -144,6 +236,7 @@  AVCodec ff_bitpacked_decoder = {
     .id = AV_CODEC_ID_BITPACKED,
     .priv_data_size        = sizeof(struct BitpackedContext),
     .init = bitpacked_init_decoder,
+    .close = bitpacked_end_decoder,
     .decode = bitpacked_decode,
     .capabilities = AV_CODEC_CAP_EXPERIMENTAL,
 };