[FFmpeg-devel] [PATCH v3 2/3] avcodec/bitpacked: add interlace support
Rostislav Pehlivanov
atomnuker at gmail.com
Tue Apr 24 23:44:12 EEST 2018
On 24 April 2018 at 19:54, Patrick Keroulas <
patrick.keroulas at savoirfairelinux.com> wrote:
> From: Damien Riegel <damien.riegel at 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 at savoirfairelinux.com>
> Signed-off-by: Patrick Keroulas <patrick.keroulas at 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.
More information about the ffmpeg-devel
mailing list