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

[Michael Niedermayer]

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 
> > > > > 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 
> 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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

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 
> > > > 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 jensFrom 66f0e12eb3e7d83113b76d8e0a71d0da328de195 Mon Sep 17 00:00:00 2001
From: Jens Ziller 
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], _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) {
+

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

[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 
> > 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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Sonntag, den 31.07.2016, 18:36 +0100 schrieb Mark Thompson:
> On 31/07/16 18:09, Jens Ziller wrote:
> > 
> > Am Sonntag, den 31.07.2016, 16:33 +0100 schrieb Mark Thompson:
> > > 
> > > Try the test stream h264/reinit-large_420_8-to-small_420_8.h264
> > > in
> > > FATE?
> > I don't test it. I tested with MPEG2 and H264 DVB-S streams. Where
> > can
> > I find this test stream?
>  small_420_8.h264>

If wight/height change the application must close the decoder and start
again with new wight/height. There is no influence to my patch.

Regards Jens
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 31/07/16 18:09, Jens Ziller wrote:
> Am Sonntag, den 31.07.2016, 16:33 +0100 schrieb Mark Thompson:
>> Try the test stream h264/reinit-large_420_8-to-small_420_8.h264 in
>> FATE?
> I don't test it. I tested with MPEG2 and H264 DVB-S streams. Where can
> I find this test stream?


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

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:
> > > 
> > > Does this also do the right thing if the decoder is reconfigured
> > > with
> > > frames outstanding?  To me (without really knowing the code) it
> > > looks
> > > like it will overwrite the old format (line 702) and therefore
> > > mess
> > > with existing frames, though there are multiple levels of
> > > indirection
> > > so the frame could be holding onto a reference by some route I'm
> > > not
> > > seeing.
> > MMAL_EVENT_FORMAT_CHANGED is set before the first decoded frame
> > give
> > out. I have never seen that this flag is set twice between
> > start/stop
> > decoder.
> Try the test stream h264/reinit-large_420_8-to-small_420_8.h264 in
> FATE?
I don't test it. I tested with MPEG2 and H264 DVB-S streams. Where can
I find this test stream?
> 
> > 
> > > 
> > > Similarly for stopping the decoder - there may be output frames
> > > which
> > > are still valid, and it looks to me like the format structure
> > > will
> > > have disappeared.  (Is there some internal reference via the
> > > MMAL_BUFFER_HEADER_T which solves both of these cases, maybe?  If
> > > so,
> > > it might be clearer if you could use that internal reference to
> > > set
> > > the value rather than going via the decoder.)
> > The struct MMAL_ES_FORMAT_T will copied in the MMAL_PORT_T struct
> > to
> > configure the component. There it lives until the application
> > change
> > anything.
> Hmm, that's not quite what I meant to ask; apologies because the
> question wasn't very clear.
Sorry, english is not my mother language.
> 
> Consider this sequence, where we want to decode a single image and
> then do something with it:
> 
> open decoder
> decode frame
> close decoder
> open 
> 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.

Regards,
Jens
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Mark Thompson]

On 31/07/16 15:51, Jens Ziller wrote:
> Am Samstag, den 30.07.2016, 17:30 +0100 schrieb Mark Thompson:
>> Does this also do the right thing if the decoder is reconfigured with
>> frames outstanding?  To me (without really knowing the code) it looks
>> like it will overwrite the old format (line 702) and therefore mess
>> with existing frames, though there are multiple levels of indirection
>> so the frame could be holding onto a reference by some route I'm not
>> seeing.
> MMAL_EVENT_FORMAT_CHANGED is set before the first decoded frame give
> out. I have never seen that this flag is set twice between start/stop
> decoder.

Try the test stream h264/reinit-large_420_8-to-small_420_8.h264 in FATE?

>> Similarly for stopping the decoder - there may be output frames which
>> are still valid, and it looks to me like the format structure will
>> have disappeared.  (Is there some internal reference via the
>> MMAL_BUFFER_HEADER_T which solves both of these cases, maybe?  If so,
>> it might be clearer if you could use that internal reference to set
>> the value rather than going via the decoder.)
> The struct MMAL_ES_FORMAT_T will copied in the MMAL_PORT_T struct to
> configure the component. There it lives until the application change
> anything.

Hmm, that's not quite what I meant to ask; apologies because the question 
wasn't very clear.

Consider this sequence, where we want to decode a single image and then do 
something with it:

open decoder
decode frame
close decoder
open 
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?

>> Also, will this structure always be available with sufficient
>> lifetime for MMAL frames produced by things other than the
>> decoder?  Your documentation on the pixfmt is phrased such that it is
>> always required - given that it appears to be for a specific use-
>> case, maybe it would be better if it were optional.
> Pixelformat AV_PIX_FMT_MMAL is a MMAL specific format. It makes only
> sense to use it with MMAL components. All MMAL components needs a
> MMAL_ES_FORMAT_T. The MMAL documented and easiest way is to copy the
> struct from decoder to the components.

Right, that makes sense.  Thank you for clarifying :)

- Mark

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Samstag, den 30.07.2016, 17:30 +0100 schrieb Mark Thompson:
> On 30/07/16 15:50, Jens Ziller wrote:
> > 
> > Am Samstag, den 16.07.2016, 17:27 +0200 schrieb Jens Ziller:
> > > 
> > > Hello Rodger and wm4,
> > > 
> > > for interlaced frames I need AVFrame->interlaced_frame. For
> > > invoke
> > > MMAL
> > > components like deinterlacer and renderer added a pointer data[2]
> > > to
> > > existing MMAL_ES_FORMAT_T. I don't have find a better solution to
> > > give
> > > a pointer to user app. I have added a patch as suggestion. Please
> > > help 
> > > me that I can release my code.
> > > 
> > > regards Jens
> > > 
> > > 
> > >  Weitergeleitete Nachricht 
> > > Von: Michael Niedermayer 
> > > Reply-to: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > An: FFmpeg development discussions and patches  > > eg.o
> > > rg
> > > > 
> > > > 
> > > > 
> > > Betreff: Re: [FFmpeg-devel] [PATCH] libavcodec/mmaldec.c: add
> > > interlaced_frame and format struct to AVFrame for deinterlacing
> > > Datum: Sat, 16 Jul 2016 14:41:50 +0200
> > > 
> > > On Sat, Jul 16, 2016 at 11:08:12AM +0200, Jens Ziller wrote:
> > > > 
> > > > 
> > > > 
> > > > Am Sonntag, den 03.07.2016, 19:36 +0200 schrieb Jens Ziller:
> > > > > 
> > > > > 
> > > > > 
> > > > > Am Sonntag, den 03.07.2016, 18:05 +0200 schrieb Moritz
> > > > > Barsnick:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz
> > > > > > > Barsnick:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +ctx->interlaced_frame =
> > > > > > > > > !(interlace_type.eMode
> > > > > > > > > ==
> > > > > > > > > MMAL_InterlaceProgressive);
> > > > > > > > What's wrong with using the "!=" operator instead?
> > > > > > > "!=" is a comparing. "= !()" assign with a negate. Here
> > > > > > > is "=
> > > > > > > !()"
> > > > > > > needed.
> > > > > > I meant the comparison, not the assignment, so replacing:
> > > > > >   ctx->interlaced_frame = !(interlace_type.eMode ==
> > > > > > MMAL_InterlaceProgressive)
> > > > > > with
> > > > > >   ctx->interlaced_frame = (interlace_type.eMode !=
> > > > > > MMAL_InterlaceProgressive)
> > > > > > 
> > > > > > The former is rather ... convoluted.
> > > > > > (Brackets optional, but probably better for readability.)
> > > > > > 
> > > > > > Moritz
> > > > > Oh, sorry! I'am so blind. Yes, that's not really smart. I
> > > > > changed
> > > > > that.
> > > > > The new patch is attached.
> > > > > 
> > > > > Regards Jens
> > > > > 
> > > > Hello Michael and list,
> > > > 
> > > > this patch was discussed and improved on the ML. What can I
> > > > still
> > > > do
> > > > that my app get interlaced_frame and a pointer to the existing
> > > > 
> > > > MMAL_ES_FORMAT_T from ffmpeg? mmaldec.c have unfortunately no
> > > > maintainer.
> > > But it has authors,
> > > you can ask rodger combs and wm4
> > > 
> > > If they do not reply then please explain why you need this
> > > structure
> > > in data[2]
> > > Also what is the lifetime of this structure and what is the
> > > liftime
> > > of the AVFrame that contains it. Its neccessary that the struct
> > > stays
> > > valid as long as the AVFrame could be
> > > 
> > > [...]
> > > 
> > Hello Michael,
> > 
> > I have ask rodger combs and wm4 two weeks ago, but unfortunately no
> > reply. Please integrate the attached patch.
> > 
> > MMAL_ES_FORMAT_T is needed for invoke the MMAL deinterlacer and
> > renderer in an application.
> > 
> > This struct give the decoder before the first frame gives out.
> > In mmaldec.c line 689 ask the flag MMAL_EVENT_FORMAT_CHANGED and if
> > this flag is set MMAL_ES_FORMAT_T is filled. This struct was
> > cleaned if
> > the decoder was stopped in mmaldec.c line 150. There are no
> > lifetime
> > issue.
> Does this also do the right thing if the decoder is reconfigured with
> frames outstanding?  To me (without really knowing the code) it looks
> like it will overwrite the old format (line 702) and therefore mess
> with existing frames, though there are multiple levels of indirection
> so the frame could be holding onto a reference by some route I'm not
> seeing.
MMAL_EVENT_FORMAT_CHANGED is set before the first decoded frame give
out. I have never seen that this flag is set twice between start/stop
decoder.

> 
> Similarly for stopping the decoder - there may be output frames which
> are still valid, and it looks to me like the format structure will
> have disappeared.  (Is there some internal reference via the
> MMAL_BUFFER_HEADER_T 

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

[Mark Thompson]

On 30/07/16 15:50, Jens Ziller wrote:
> Am Samstag, den 16.07.2016, 17:27 +0200 schrieb Jens Ziller:
>> Hello Rodger and wm4,
>>
>> for interlaced frames I need AVFrame->interlaced_frame. For invoke
>> MMAL
>> components like deinterlacer and renderer added a pointer data[2] to
>> existing MMAL_ES_FORMAT_T. I don't have find a better solution to
>> give
>> a pointer to user app. I have added a patch as suggestion. Please
>> help 
>> me that I can release my code.
>>
>> regards Jens
>>
>>
>>  Weitergeleitete Nachricht 
>> Von: Michael Niedermayer 
>> Reply-to: FFmpeg development discussions and patches > de...@ffmpeg.org>
>> An: FFmpeg development discussions and patches > rg
>>>
>>>
>> Betreff: Re: [FFmpeg-devel] [PATCH] libavcodec/mmaldec.c: add
>> interlaced_frame and format struct to AVFrame for deinterlacing
>> Datum: Sat, 16 Jul 2016 14:41:50 +0200
>>
>> On Sat, Jul 16, 2016 at 11:08:12AM +0200, Jens Ziller wrote:
>>>
>>>
>>> Am Sonntag, den 03.07.2016, 19:36 +0200 schrieb Jens Ziller:


 Am Sonntag, den 03.07.2016, 18:05 +0200 schrieb Moritz Barsnick:
>
>
>
> On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
>>
>>
>>
>>
>> Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz
>> Barsnick:
>>>
>>>
>>>
>>>
>>> On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:





 +ctx->interlaced_frame = !(interlace_type.eMode
 ==
 MMAL_InterlaceProgressive);
>>> What's wrong with using the "!=" operator instead?
>> "!=" is a comparing. "= !()" assign with a negate. Here is "=
>> !()"
>> needed.
> I meant the comparison, not the assignment, so replacing:
>   ctx->interlaced_frame = !(interlace_type.eMode ==
> MMAL_InterlaceProgressive)
> with
>   ctx->interlaced_frame = (interlace_type.eMode !=
> MMAL_InterlaceProgressive)
>
> The former is rather ... convoluted.
> (Brackets optional, but probably better for readability.)
>
> Moritz
 Oh, sorry! I'am so blind. Yes, that's not really smart. I changed
 that.
 The new patch is attached.

 Regards Jens

>>> Hello Michael and list,
>>>
>>> this patch was discussed and improved on the ML. What can I still
>>> do
>>> that my app get interlaced_frame and a pointer to the existing
>>>
>>> MMAL_ES_FORMAT_T from ffmpeg? mmaldec.c have unfortunately no
>>> maintainer.
>> But it has authors,
>> you can ask rodger combs and wm4
>>
>> If they do not reply then please explain why you need this structure
>> in data[2]
>> Also what is the lifetime of this structure and what is the liftime
>> of the AVFrame that contains it. Its neccessary that the struct stays
>> valid as long as the AVFrame could be
>>
>> [...]
>>
> 
> Hello Michael,
> 
> I have ask rodger combs and wm4 two weeks ago, but unfortunately no
> reply. Please integrate the attached patch.
> 
> MMAL_ES_FORMAT_T is needed for invoke the MMAL deinterlacer and
> renderer in an application.
> 
> This struct give the decoder before the first frame gives out.
> In mmaldec.c line 689 ask the flag MMAL_EVENT_FORMAT_CHANGED and if
> this flag is set MMAL_ES_FORMAT_T is filled. This struct was cleaned if
> the decoder was stopped in mmaldec.c line 150. There are no lifetime
> issue.

Does this also do the right thing if the decoder is reconfigured with frames 
outstanding?  To me (without really knowing the code) it looks like it will 
overwrite the old format (line 702) and therefore mess with existing frames, 
though there are multiple levels of indirection so the frame could be holding 
onto a reference by some route I'm not seeing.

Similarly for stopping the decoder - there may be output frames which are still 
valid, and it looks to me like the format structure will have disappeared.  (Is 
there some internal reference via the MMAL_BUFFER_HEADER_T which solves both of 
these cases, maybe?  If so, it might be clearer if you could use that internal 
reference to set the value rather than going via the decoder.)

Also, will this structure always be available with sufficient lifetime for MMAL 
frames produced by things other than the decoder?  Your documentation on the 
pixfmt is phrased such that it is always required - given that it appears to be 
for a specific use-case, maybe it would be better if it were optional.

Thanks,

- Mark

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Samstag, den 16.07.2016, 17:27 +0200 schrieb Jens Ziller:
> Hello Rodger and wm4,
> 
> for interlaced frames I need AVFrame->interlaced_frame. For invoke
> MMAL
> components like deinterlacer and renderer added a pointer data[2] to
> existing MMAL_ES_FORMAT_T. I don't have find a better solution to
> give
> a pointer to user app. I have added a patch as suggestion. Please
> help 
> me that I can release my code.
> 
> regards Jens
> 
> 
>  Weitergeleitete Nachricht 
> Von: Michael Niedermayer 
> Reply-to: FFmpeg development discussions and patches  de...@ffmpeg.org>
> An: FFmpeg development discussions and patches  rg
> > 
> > 
> Betreff: Re: [FFmpeg-devel] [PATCH] libavcodec/mmaldec.c: add
> interlaced_frame and format struct to AVFrame for deinterlacing
> Datum: Sat, 16 Jul 2016 14:41:50 +0200
> 
> On Sat, Jul 16, 2016 at 11:08:12AM +0200, Jens Ziller wrote:
> > 
> > 
> > Am Sonntag, den 03.07.2016, 19:36 +0200 schrieb Jens Ziller:
> > > 
> > > 
> > > Am Sonntag, den 03.07.2016, 18:05 +0200 schrieb Moritz Barsnick:
> > > > 
> > > > 
> > > > 
> > > > On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz
> > > > > Barsnick:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +ctx->interlaced_frame = !(interlace_type.eMode
> > > > > > > ==
> > > > > > > MMAL_InterlaceProgressive);
> > > > > > What's wrong with using the "!=" operator instead?
> > > > > "!=" is a comparing. "= !()" assign with a negate. Here is "=
> > > > > !()"
> > > > > needed.
> > > > I meant the comparison, not the assignment, so replacing:
> > > >   ctx->interlaced_frame = !(interlace_type.eMode ==
> > > > MMAL_InterlaceProgressive)
> > > > with
> > > >   ctx->interlaced_frame = (interlace_type.eMode !=
> > > > MMAL_InterlaceProgressive)
> > > > 
> > > > The former is rather ... convoluted.
> > > > (Brackets optional, but probably better for readability.)
> > > > 
> > > > Moritz
> > > Oh, sorry! I'am so blind. Yes, that's not really smart. I changed
> > > that.
> > > The new patch is attached.
> > > 
> > > Regards Jens
> > > 
> > Hello Michael and list,
> > 
> > this patch was discussed and improved on the ML. What can I still
> > do
> > that my app get interlaced_frame and a pointer to the existing
> > 
> > MMAL_ES_FORMAT_T from ffmpeg? mmaldec.c have unfortunately no
> > maintainer.
> But it has authors,
> you can ask rodger combs and wm4
> 
> If they do not reply then please explain why you need this structure
> in data[2]
> Also what is the lifetime of this structure and what is the liftime
> of the AVFrame that contains it. Its neccessary that the struct stays
> valid as long as the AVFrame could be
> 
> [...]
> 

Hello Michael,

I have ask rodger combs and wm4 two weeks ago, but unfortunately no
reply. Please integrate the attached patch.

MMAL_ES_FORMAT_T is needed for invoke the MMAL deinterlacer and
renderer in an application.

This struct give the decoder before the first frame gives out.
In mmaldec.c line 689 ask the flag MMAL_EVENT_FORMAT_CHANGED and if
this flag is set MMAL_ES_FORMAT_T is filled. This struct was cleaned if
the decoder was stopped in mmaldec.c line 150. There are no lifetime
issue.

regards jensFrom b724bbe3a13addb055da883cbe802eee74d7c65e Mon Sep 17 00:00:00 2001
From: Jens Ziller 
Date: Sun, 3 Jul 2016 19:25:23 +0200
Subject: [PATCH] v4 fill AVFrame->interlaced_frame with
 MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T, add a pointer data[2] to
 MMAL_ES_FORMAT_T that user application can invoke MMAL components like
 deinterlacer and renderer with it

---
 libavcodec/mmaldec.c | 17 +
 libavutil/pixfmt.h   |  3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..2e7b43c 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,7 @@ typedef struct MMALDecodeContext {
 int eos_received;
 int eos_sent;
 int extradata_sent;
+int interlaced_frame;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +275,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 +302,15 @@ 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;
+

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

[Michael Niedermayer]

On Sat, Jul 16, 2016 at 11:08:12AM +0200, Jens Ziller wrote:
> Am Sonntag, den 03.07.2016, 19:36 +0200 schrieb Jens Ziller:
> > Am Sonntag, den 03.07.2016, 18:05 +0200 schrieb Moritz Barsnick:
> > > 
> > > On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
> > > > 
> > > > 
> > > > Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz Barsnick:
> > > > > 
> > > > > 
> > > > > On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +ctx->interlaced_frame = !(interlace_type.eMode ==
> > > > > > MMAL_InterlaceProgressive);
> > > > > What's wrong with using the "!=" operator instead?
> > > > "!=" is a comparing. "= !()" assign with a negate. Here is "=
> > > > !()"
> > > > needed.
> > > I meant the comparison, not the assignment, so replacing:
> > >   ctx->interlaced_frame = !(interlace_type.eMode ==
> > > MMAL_InterlaceProgressive)
> > > with
> > >   ctx->interlaced_frame = (interlace_type.eMode !=
> > > MMAL_InterlaceProgressive)
> > > 
> > > The former is rather ... convoluted.
> > > (Brackets optional, but probably better for readability.)
> > > 
> > > Moritz
> > Oh, sorry! I'am so blind. Yes, that's not really smart. I changed
> > that.
> > The new patch is attached.
> > 
> > Regards Jens
> > 
> 
> Hello Michael and list,
> 
> this patch was discussed and improved on the ML. What can I still do
> that my app get interlaced_frame and a pointer to the existing

> MMAL_ES_FORMAT_T from ffmpeg? mmaldec.c have unfortunately no
> maintainer.

But it has authors,
you can ask rodger combs and wm4

If they do not reply then please explain why you need this structure
in data[2]
Also what is the lifetime of this structure and what is the liftime
of the AVFrame that contains it. Its neccessary that the struct stays
valid as long as the AVFrame could be

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Sonntag, den 03.07.2016, 18:05 +0200 schrieb Moritz Barsnick:
> On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
> > 
> > Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz Barsnick:
> > > 
> > > On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> > > > 
> > > > 
> > > > +ctx->interlaced_frame = !(interlace_type.eMode ==
> > > > MMAL_InterlaceProgressive);
> > > What's wrong with using the "!=" operator instead?
> > "!=" is a comparing. "= !()" assign with a negate. Here is "= !()"
> > needed.
> I meant the comparison, not the assignment, so replacing:
>   ctx->interlaced_frame = !(interlace_type.eMode ==
> MMAL_InterlaceProgressive)
> with
>   ctx->interlaced_frame = (interlace_type.eMode !=
> MMAL_InterlaceProgressive)
> 
> The former is rather ... convoluted.
> (Brackets optional, but probably better for readability.)
> 
> Moritz

Oh, sorry! I'am so blind. Yes, that's not really smart. I changed that.
The new patch is attached.

Regards Jens

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-develFrom b724bbe3a13addb055da883cbe802eee74d7c65e Mon Sep 17 00:00:00 2001
From: Jens Ziller 
Date: Sun, 3 Jul 2016 19:25:23 +0200
Subject: [PATCH] v4 fill AVFrame->interlaced_frame with
 MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T, add a pointer data[2] to
 MMAL_ES_FORMAT_T that user application can invoke MMAL components like
 deinterlacer and renderer with it

---
 libavcodec/mmaldec.c | 17 +
 libavutil/pixfmt.h   |  3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..2e7b43c 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,7 @@ typedef struct MMALDecodeContext {
 int eos_received;
 int eos_sent;
 int extradata_sent;
+int interlaced_frame;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +275,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 +302,15 @@ 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], _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);
+}
+
 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;
@@ -609,7 +620,13 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
 MMALDecodeContext *ctx = avctx->priv_data;
 int ret = 0;
 
+frame->interlaced_frame = ctx->interlaced_frame;
+
 if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
+
+// in data[2] give the format struct for configure deinterlacer and renderer
+frame->data[2] = ctx->decoder->output[0]->format;
+
 if (!ctx->pool_out)
 return AVERROR_UNKNOWN; // format change code failed with OOM previously
 
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index 0ed01c4..98982f8 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -235,7 +235,8 @@ enum AVPixelFormat {
 AV_PIX_FMT_QSV,
 /**
  * HW acceleration though MMAL, data[3] contains a pointer to the
- * MMAL_BUFFER_HEADER_T structure.
+ * MMAL_BUFFER_HEADER_T structure and data[2] contains a pointer to the
+ * MMAL_ES_FORMAT_T structure.
  */
 AV_PIX_FMT_MMAL,
 
-- 
2.7.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Moritz Barsnick]

On Sun, Jul 03, 2016 at 17:20:41 +0200, Jens Ziller wrote:
> Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz Barsnick:
> > On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> > > 
> > > +ctx->interlaced_frame = !(interlace_type.eMode == 
> > > MMAL_InterlaceProgressive);
> > What's wrong with using the "!=" operator instead?
> 
> "!=" is a comparing. "= !()" assign with a negate. Here is "= !()" needed.

I meant the comparison, not the assignment, so replacing:
  ctx->interlaced_frame = !(interlace_type.eMode == MMAL_InterlaceProgressive)
with
  ctx->interlaced_frame = (interlace_type.eMode != MMAL_InterlaceProgressive)

The former is rather ... convoluted.
(Brackets optional, but probably better for readability.)

Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Samstag, den 02.07.2016, 17:54 +0200 schrieb Moritz Barsnick:
> On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> > 
> > +ctx->interlaced_frame = !(interlace_type.eMode ==
> > MMAL_InterlaceProgressive);
> What's wrong with using the "!=" operator instead?

"!=" is a comparing. "= !()" assign with a negate. Here is "= !()"
needed.

> 
> > 
> >  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
> >  if (!ctx->pool_out)
> > +// in data[2] give the format struct for configure
> > deinterlacer and renderer
> > +frame->data[2] = ctx->decoder->output[0]->format;
> Incorrect indentation.

My fault.

The new patch is attached. Please integrate the patch. That I can
release my App who deal with it.

Regards Jens

> 
> Moritz
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-develFrom 8c8700c6277a73e6c9d0d02d2afbbf8b0e75213e Mon Sep 17 00:00:00 2001
From: Jens Ziller 
Date: Sun, 3 Jul 2016 17:05:35 +0200
Subject: [PATCH] v3 fill AVFrame->interlaced_frame with
 MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T, add a pointer data[2] to
 MMAL_ES_FORMAT_T that user application can invoke MMAL components like
 deinterlacer and renderer with it

---
 libavcodec/mmaldec.c | 17 +
 libavutil/pixfmt.h   |  3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..c4bf414 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,7 @@ typedef struct MMALDecodeContext {
 int eos_received;
 int eos_sent;
 int extradata_sent;
+int interlaced_frame;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +275,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 +302,15 @@ 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], _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);
+}
+
 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;
@@ -609,7 +620,13 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
 MMALDecodeContext *ctx = avctx->priv_data;
 int ret = 0;
 
+frame->interlaced_frame = ctx->interlaced_frame;
+
 if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
+
+// in data[2] give the format struct for configure deinterlacer and renderer
+frame->data[2] = ctx->decoder->output[0]->format;
+
 if (!ctx->pool_out)
 return AVERROR_UNKNOWN; // format change code failed with OOM previously
 
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index 0ed01c4..98982f8 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -235,7 +235,8 @@ enum AVPixelFormat {
 AV_PIX_FMT_QSV,
 /**
  * HW acceleration though MMAL, data[3] contains a pointer to the
- * MMAL_BUFFER_HEADER_T structure.
+ * MMAL_BUFFER_HEADER_T structure and data[2] contains a pointer to the
+ * MMAL_ES_FORMAT_T structure.
  */
 AV_PIX_FMT_MMAL,
 
-- 
2.7.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Moritz Barsnick]

On Sun, Jun 26, 2016 at 17:12:14 +0200, Jens Ziller wrote:
> +ctx->interlaced_frame = !(interlace_type.eMode == 
> MMAL_InterlaceProgressive);

What's wrong with using the "!=" operator instead?

>  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
>  if (!ctx->pool_out)
> +// in data[2] give the format struct for configure deinterlacer and 
> renderer
> +frame->data[2] = ctx->decoder->output[0]->format;

Incorrect indentation.

Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Sonntag, den 26.06.2016, 18:08 +0200 schrieb Hendrik Leppkes:
> On Sun, Jun 26, 2016 at 5:12 PM, Jens Ziller  wrote:
> > 
> > Am Samstag, den 25.06.2016, 12:52 +0200 schrieb Michael
> > Niedermayer:
> > > 
> > > On Fri, Jun 24, 2016 at 06:27:38PM +0200, Jens Ziller wrote:
> > > > 
> > > > 
> > > > Hello,
> > > > 
> > > > deinterlacing need frame->interlaced_frame and format struct.
> > > > This
> > > > patch added this to AVFrame.
> > > > 
> > > > Regards Jens.
> > > > 
> > > >  mmaldec.c |   16 
> > > >  1 file changed, 16 insertions(+)
> > > > 6351a54c36d98d1f6ffdaeea96af8c0db1305358  0001-for-
> > > > deinterlacing-
> > > > needed.patch
> > > > From 8a8961a4fab0da2bd98ef6cbfaf55462a00d3450 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > From: Jens Ziller 
> > > > Date: Fri, 24 Jun 2016 18:18:12 +0200
> > > > 
> > > > Subject: [PATCH] for deinterlacing needed
> > > This commit message is not ok
> > > 
> > > The message should describe
> > > 1. what is changed
> > > 2. why it is changed
> > > 3. how it is changed
> > > 
> > > When in doubt always write a longer commit message than a short
> > > one
> > > 
> > > [...]
> > > > 
> > > > 
> > > >  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
> > > >  if (!ctx->pool_out)
> > > > +// in data[2] give the format struct for configure
> > > > deinterlacer and renderer
> > > > +frame->data[2] = ctx->decoder->output[0]->format;
> > > This is not how AV_PIX_FMT_MMAL is documented:
> > > 
> > > /**
> > >  * HW acceleration though MMAL, data[3] contains a pointer to
> > > the
> > >  * MMAL_BUFFER_HEADER_T structure.
> > >  */
> > > AV_PIX_FMT_MMAL,
> > > 
> > > also where is the code that uses data[2] ?
> > > 
> > > [...]
> > > 
> > Attached is the new Version. Hints and comments are welcome.
> > 
> > Regards Jens.
> > 
> MMAL_ES_FORMAT_T looks like it contains information about the encoded
> stream, and pretty trivial information at that - things that are in
> AVCodecContext anyway.
> Maybe whoever needs such a struct should just re-assemble it instead
> of re-definining the pixel format definition here?
> 
> - Hendrik
> 
Yes, AVCodecContext is similise to MMAL_ES_FORMAT_T. But copy the data
from MMAL_ES_FORMAT_T to AVCodecContext and the user app copy the data
to a new structure MMAL_ES_FORMAT_T is not smart. I think give a
pointer from the existing MMAL_ES_FORMAT_T to user app is nicer. I
don't have find a better solution to give a pointer to user app.
It's not a re-definining. It's an addition. The structure
MMAL_ES_FORMAT_T is only used with AV_PIX_FMT_MMAL format. MMAL is
RaspberryPi specific.

Regards Jens.

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Sun, Jun 26, 2016 at 5:12 PM, Jens Ziller  wrote:
> Am Samstag, den 25.06.2016, 12:52 +0200 schrieb Michael Niedermayer:
>> On Fri, Jun 24, 2016 at 06:27:38PM +0200, Jens Ziller wrote:
>> >
>> > Hello,
>> >
>> > deinterlacing need frame->interlaced_frame and format struct. This
>> > patch added this to AVFrame.
>> >
>> > Regards Jens.
>> >
>> >  mmaldec.c |   16 
>> >  1 file changed, 16 insertions(+)
>> > 6351a54c36d98d1f6ffdaeea96af8c0db1305358  0001-for-deinterlacing-
>> > needed.patch
>> > From 8a8961a4fab0da2bd98ef6cbfaf55462a00d3450 Mon Sep 17 00:00:00
>> > 2001
>> > From: Jens Ziller 
>> > Date: Fri, 24 Jun 2016 18:18:12 +0200
>> >
>> > Subject: [PATCH] for deinterlacing needed
>> This commit message is not ok
>>
>> The message should describe
>> 1. what is changed
>> 2. why it is changed
>> 3. how it is changed
>>
>> When in doubt always write a longer commit message than a short one
>>
>> [...]
>> >
>> >  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
>> >  if (!ctx->pool_out)
>> > +// in data[2] give the format struct for configure
>> > deinterlacer and renderer
>> > +frame->data[2] = ctx->decoder->output[0]->format;
>> This is not how AV_PIX_FMT_MMAL is documented:
>>
>> /**
>>  * HW acceleration though MMAL, data[3] contains a pointer to the
>>  * MMAL_BUFFER_HEADER_T structure.
>>  */
>> AV_PIX_FMT_MMAL,
>>
>> also where is the code that uses data[2] ?
>>
>> [...]
>>
>
> Attached is the new Version. Hints and comments are welcome.
>
> Regards Jens.
>

MMAL_ES_FORMAT_T looks like it contains information about the encoded
stream, and pretty trivial information at that - things that are in
AVCodecContext anyway.
Maybe whoever needs such a struct should just re-assemble it instead
of re-definining the pixel format definition here?

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Am Samstag, den 25.06.2016, 12:52 +0200 schrieb Michael Niedermayer:
> On Fri, Jun 24, 2016 at 06:27:38PM +0200, Jens Ziller wrote:
> > 
> > Hello,
> > 
> > deinterlacing need frame->interlaced_frame and format struct. This
> > patch added this to AVFrame.
> > 
> > Regards Jens.
> > 
> >  mmaldec.c |   16 
> >  1 file changed, 16 insertions(+)
> > 6351a54c36d98d1f6ffdaeea96af8c0db1305358  0001-for-deinterlacing-
> > needed.patch
> > From 8a8961a4fab0da2bd98ef6cbfaf55462a00d3450 Mon Sep 17 00:00:00
> > 2001
> > From: Jens Ziller 
> > Date: Fri, 24 Jun 2016 18:18:12 +0200
> > 
> > Subject: [PATCH] for deinterlacing needed
> This commit message is not ok
> 
> The message should describe
> 1. what is changed
> 2. why it is changed
> 3. how it is changed
> 
> When in doubt always write a longer commit message than a short one
> 
> [...]
> > 
> >  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
> >  if (!ctx->pool_out)
> > +// in data[2] give the format struct for configure
> > deinterlacer and renderer
> > +frame->data[2] = ctx->decoder->output[0]->format;
> This is not how AV_PIX_FMT_MMAL is documented:
> 
> /**
>  * HW acceleration though MMAL, data[3] contains a pointer to the
>  * MMAL_BUFFER_HEADER_T structure.
>  */
> AV_PIX_FMT_MMAL,
> 
> also where is the code that uses data[2] ?
> 
> [...]
> 

Attached is the new Version. Hints and comments are welcome.

Regards Jens.

> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-develFrom eb6b4d03b258789b4b7baafe89fec1a8feae00f7 Mon Sep 17 00:00:00 2001
From: Jens Ziller 
Date: Sun, 26 Jun 2016 17:01:34 +0200
Subject: [PATCH] v2 fill AVFrame->interlaced_frame with
 MMAL_PARAMETER_VIDEO_INTERLACE_TYPE_T, add a pointer data[2] to
 MMAL_ES_FORMAT_T that user application can invoke MMAL components like
 deinterlacer and renderer with it

---
 libavcodec/mmaldec.c | 16 
 libavutil/pixfmt.h   |  3 ++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..b38a3bd 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,7 @@ typedef struct MMALDecodeContext {
 int eos_received;
 int eos_sent;
 int extradata_sent;
+int interlaced_frame;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +275,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 +302,15 @@ 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], _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);
+}
+
 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;
@@ -609,8 +620,13 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
 MMALDecodeContext *ctx = avctx->priv_data;
 int ret = 0;
 
+frame->interlaced_frame = ctx->interlaced_frame;
+
 if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
 if (!ctx->pool_out)
+// in data[2] give the format struct for configure deinterlacer and renderer
+frame->data[2] = ctx->decoder->output[0]->format;
+
 return AVERROR_UNKNOWN; // format change code failed with OOM previously
 
 if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index 0ed01c4..98982f8 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -235,7 +235,8 @@ enum AVPixelFormat {
 AV_PIX_FMT_QSV,
 /**
  * HW acceleration though MMAL, data[3] contains a pointer to the
- * MMAL_BUFFER_HEADER_T structure.
+ * MMAL_BUFFER_HEADER_T structure and data[2] contains a pointer to the
+ * MMAL_ES_FORMAT_T structure.
  */
 AV_PIX_FMT_MMAL,
 
-- 
2.7.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Fri, Jun 24, 2016 at 06:27:38PM +0200, Jens Ziller wrote:
> Hello,
> 
> deinterlacing need frame->interlaced_frame and format struct. This
> patch added this to AVFrame.
> 
> Regards Jens.

>  mmaldec.c |   16 
>  1 file changed, 16 insertions(+)
> 6351a54c36d98d1f6ffdaeea96af8c0db1305358  0001-for-deinterlacing-needed.patch
> From 8a8961a4fab0da2bd98ef6cbfaf55462a00d3450 Mon Sep 17 00:00:00 2001
> From: Jens Ziller 
> Date: Fri, 24 Jun 2016 18:18:12 +0200

> Subject: [PATCH] for deinterlacing needed

This commit message is not ok

The message should describe
1. what is changed
2. why it is changed
3. how it is changed

When in doubt always write a longer commit message than a short one


[...]
>  if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
>  if (!ctx->pool_out)
> +// in data[2] give the format struct for configure deinterlacer and 
> renderer
> +frame->data[2] = ctx->decoder->output[0]->format;

This is not how AV_PIX_FMT_MMAL is documented:

/**
 * HW acceleration though MMAL, data[3] contains a pointer to the
 * MMAL_BUFFER_HEADER_T structure.
 */
AV_PIX_FMT_MMAL,

also where is the code that uses data[2] ?

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jens Ziller]

Hello,

deinterlacing need frame->interlaced_frame and format struct. This
patch added this to AVFrame.

Regards Jens.From 8a8961a4fab0da2bd98ef6cbfaf55462a00d3450 Mon Sep 17 00:00:00 2001
From: Jens Ziller 
Date: Fri, 24 Jun 2016 18:18:12 +0200
Subject: [PATCH] for deinterlacing needed

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

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 099a8c5..b38a3bd 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -88,6 +88,7 @@ typedef struct MMALDecodeContext {
 int eos_received;
 int eos_sent;
 int extradata_sent;
+int interlaced_frame;
 } MMALDecodeContext;
 
 // Assume decoder is guaranteed to produce output after at least this many
@@ -274,6 +275,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 +302,15 @@ 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], _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);
+}
+
 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;
@@ -609,8 +620,13 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
 MMALDecodeContext *ctx = avctx->priv_data;
 int ret = 0;
 
+frame->interlaced_frame = ctx->interlaced_frame;
+
 if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
 if (!ctx->pool_out)
+// in data[2] give the format struct for configure deinterlacer and renderer
+frame->data[2] = ctx->decoder->output[0]->format;
+
 return AVERROR_UNKNOWN; // format change code failed with OOM previously
 
 if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
-- 
2.7.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel