On 14 May 2018 at 20:29, Patrick Keroulas <patrick.keroulas@
savoirfairelinux.com> wrote:
> From: Damien Riegel <damien.rie...@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.kerou...@savoirfairelinux.com>
> Signed-off-by: Damien Riegel <damien.rie...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
