diff mbox

[FFmpeg-devel] libavcodec/mmaldec.c: add interlaced_frame and format struct to AVFrame for deinterlacing]

Message ID 1471186923.1575.7.camel@gmx.de
State Accepted
Headers show

Commit Message

Jens Ziller Aug. 14, 2016, 3:02 p.m. UTC 
Am Samstag, den 13.08.2016, 13:16 +0200 schrieb Jens Ziller:
> Am Freitag, den 12.08.2016, 17:12 +0200 schrieb Michael Niedermayer:
> > 
> > On Sun, Jul 31, 2016 at 07:09:26PM +0200, Jens Ziller wrote:
> > > 
> > > 
> > > Am Sonntag, den 31.07.2016, 16:33 +0100 schrieb Mark Thompson:
> > > > 
> > > > 
> > > > On 31/07/16 15:51, Jens Ziller wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > Am Samstag, den 30.07.2016, 17:30 +0100 schrieb Mark
> > > > > Thompson:
> > [...]
> > > 
> > > 
> > > > 
> > > > 
> > > > Consider this sequence, where we want to decode a single image
> > > > and
> > > > then do something with it:
> > > > 
> > > > open decoder
> > > > decode frame
> > > > close decoder
> > > > open <some other component>
> > > > give it the frame we got from the decoder
> > > > 
> > > > To me that looks like it will invoke undefined behaviour
> > > > (segfault or
> > > > read garbage) when trying to access data[2] in the second
> > > > component,
> > > > because the pointer appears to be to the MMAL_ES_FORMAT_T in
> > > > the
> > > > MMAL_PORT_T on the decoder which we just destroyed.  If not,
> > > > where is
> > > > the reference which keeps that pointer valid living?
> > > With MMAL decoder it works:
> > > 
> > > - configure decoder
> > > - send many frames (in my tests between 5 and 20+) to decoder
> > > - decoder give MMAL_ES_FORMAT_T
> > > - configure decoder output port with given struct <- here we have
> > > the
> > > pointer
> > > - decoder send the first decoded frame
> > > 
> > > The struct lives before the first frame is available. Decode a
> > > single
> > > frame is a spezial thing. The MMAL decoder is not made for this.
> > > 
> > > A
> > > local copy from format struct make no sense for me.
> > well, it makes sense to us for the code to work and not have
> > undefined
> > behavior.
> > Please correct me if iam wrong but Hendrik, Mark and me are saying
> > the same thing more or less, i think you should change the patch
> > 
> > also additionally, its nice if we can keep data[0..2] available for
> > the raw 3 YUV planes, it might one day somewhere be nice to
> > download
> > the raw data into [0..2], using up the 2nd for this struct is not
> > pretty
> For YUV planes AV_PIX_FMT_YUV420P are the better choice
> not AV_PIX_FMT_MMAL. 
> But you have me convinced that I write a new patch, test it and send
> it
> to the ml.
> 
Here are the new version. No data[2] pointer more. For this AVFrame
top_field_first and AVCodecContext->framerate is used.

regards jens

Comments

Michael Niedermayer Sept. 11, 2016, 11:17 p.m. UTC | #1 
On Sun, Aug 14, 2016 at 05:02:03PM +0200, Jens Ziller wrote:
> Am Samstag, den 13.08.2016, 13:16 +0200 schrieb Jens Ziller:
> > Am Freitag, den 12.08.2016, 17:12 +0200 schrieb Michael Niedermayer:
> > > 
> > > On Sun, Jul 31, 2016 at 07:09:26PM +0200, Jens Ziller wrote:
> > > > 
> > > > 
> > > > Am Sonntag, den 31.07.2016, 16:33 +0100 schrieb Mark Thompson:
> > > > > 
> > > > > 
> > > > > On 31/07/16 15:51, Jens Ziller wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Am Samstag, den 30.07.2016, 17:30 +0100 schrieb Mark
> > > > > > Thompson:
> > > [...]
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > Consider this sequence, where we want to decode a single image
> > > > > and
> > > > > then do something with it:
> > > > > 
> > > > > open decoder
> > > > > decode frame
> > > > > close decoder
> > > > > open <some other component>
> > > > > give it the frame we got from the decoder
> > > > > 
> > > > > To me that looks like it will invoke undefined behaviour
> > > > > (segfault or
> > > > > read garbage) when trying to access data[2] in the second
> > > > > component,
> > > > > because the pointer appears to be to the MMAL_ES_FORMAT_T in
> > > > > the
> > > > > MMAL_PORT_T on the decoder which we just destroyed.  If not,
> > > > > where is
> > > > > the reference which keeps that pointer valid living?
> > > > With MMAL decoder it works:
> > > > 
> > > > - configure decoder
> > > > - send many frames (in my tests between 5 and 20+) to decoder
> > > > - decoder give MMAL_ES_FORMAT_T
> > > > - configure decoder output port with given struct <- here we have
> > > > the
> > > > pointer
> > > > - decoder send the first decoded frame
> > > > 
> > > > The struct lives before the first frame is available. Decode a
> > > > single
> > > > frame is a spezial thing. The MMAL decoder is not made for this.
> > > > 
> > > > A
> > > > local copy from format struct make no sense for me.
> > > well, it makes sense to us for the code to work and not have
> > > undefined
> > > behavior.
> > > Please correct me if iam wrong but Hendrik, Mark and me are saying
> > > the same thing more or less, i think you should change the patch
> > > 
> > > also additionally, its nice if we can keep data[0..2] available for
> > > the raw 3 YUV planes, it might one day somewhere be nice to
> > > download
> > > the raw data into [0..2], using up the 2nd for this struct is not
> > > pretty
> > For YUV planes AV_PIX_FMT_YUV420P are the better choice
> > not AV_PIX_FMT_MMAL. 
> > But you have me convinced that I write a new patch, test it and send
> > it
> > to the ml.
> > 
> Here are the new version. No data[2] pointer more. For this AVFrame
> top_field_first and AVCodecContext->framerate is used.
> 
> regards jens

>  mmaldec.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> e1e1ab964f9024ea523085fe7ddca4549601d51a  0001-v5-fill-AVFrame-interlaced_frame-and-top_field_first.patch
> From 66f0e12eb3e7d83113b76d8e0a71d0da328de195 Mon Sep 17 00:00:00 2001
> From: Jens Ziller <zillevdr@gmx.de>
> Date: Sun, 14 Aug 2016 16:44:39 +0200
> Subject: [PATCH] v5 fill AVFrame->interlaced_frame and top_field_first with
>  MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T and AVCodecContext->framerate from
>  MMAL_ES_FORMAT_T for deinterlacer and renderer

applied

thanks

[...]
diff mbox

Patch

From 66f0e12eb3e7d83113b76d8e0a71d0da328de195 Mon Sep 17 00:00:00 2001
From: Jens Ziller <zillevdr@gmx.de>
Date: Sun, 14 Aug 2016 16:44:39 +0200
Subject: [PATCH] v5 fill AVFrame->interlaced_frame and top_field_first with
 MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T and AVCodecContext->framerate from
 MMAL_ES_FORMAT_T for deinterlacer and renderer

---
 libavcodec/mmaldec.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..56ad948 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,8 @@  typedef struct MMALDecodeContext {
     int eos_received;
     int eos_sent;
     int extradata_sent;
+    int interlaced_frame;
+    int top_field_first;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +276,7 @@  static int ffmal_update_format(AVCodecContext *avctx)
     int ret = 0;
     MMAL_COMPONENT_T *decoder = ctx->decoder;
     MMAL_ES_FORMAT_T *format_out = decoder->output[0]->format;
+    MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T interlace_type;
 
     ffmmal_poolref_unref(ctx->pool_out);
     if (!(ctx->pool_out = av_mallocz(sizeof(*ctx->pool_out)))) {
@@ -300,6 +303,16 @@  static int ffmal_update_format(AVCodecContext *avctx)
     if ((status = mmal_port_format_commit(decoder->output[0])))
         goto fail;
 
+    interlace_type.hdr.id = MMAL_PARAMETER_VIDEO_INTERLACE_TYPE;
+    interlace_type.hdr.size = sizeof(MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T);
+    status = mmal_port_parameter_get(decoder->output[0], &interlace_type.hdr);
+    if (status != MMAL_SUCCESS) {
+        av_log(avctx, AV_LOG_ERROR, "Cannot read MMAL interlace information!\n");
+    } else {
+        ctx->interlaced_frame = (interlace_type.eMode != MMAL_InterlaceProgressive);
+        ctx->top_field_first = (interlace_type.eMode == MMAL_InterlaceFieldsInterleavedUpperFirst);
+    }
+
     if ((ret = ff_set_dimensions(avctx, format_out->es->video.crop.x + format_out->es->video.crop.width,
                                         format_out->es->video.crop.y + format_out->es->video.crop.height)) < 0)
         goto fail;
@@ -308,6 +321,10 @@  static int ffmal_update_format(AVCodecContext *avctx)
         avctx->sample_aspect_ratio.num = format_out->es->video.par.num;
         avctx->sample_aspect_ratio.den = format_out->es->video.par.den;
     }
+    if (format_out->es->video.frame_rate.num && format_out->es->video.frame_rate.den) {
+        avctx->framerate.num = format_out->es->video.frame_rate.num;
+        avctx->framerate.den = format_out->es->video.frame_rate.den;
+    }
 
     avctx->colorspace = ffmmal_csp_to_av_csp(format_out->es->video.color_space);
 
@@ -609,6 +626,9 @@  static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
     MMALDecodeContext *ctx = avctx->priv_data;
     int ret = 0;
 
+    frame->interlaced_frame = ctx->interlaced_frame;
+    frame->top_field_first = ctx->top_field_first;
+
     if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
         if (!ctx->pool_out)
             return AVERROR_UNKNOWN; // format change code failed with OOM previously
-- 
2.7.3