| Message ID | 1469890252.1521.28.camel@gmx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <michael@niedermayer.cc> >> Reply-to: FFmpeg development discussions and patches <ffmpeg- >> devel@ffmpeg.org> >> An: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.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. 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
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 <michael@niedermayer.cc> > > > Reply-to: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > An: FFmpeg development discussions and patches <ffmpeg-devel@ffmp > > > 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 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. > > 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. Regards, Jens > > Thanks, > > - Mark > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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 <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? >> 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
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 <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. Regards, Jens
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? <http://fate-suite.ffmpeg.org/h264/reinit-large_420_8-to-small_420_8.h264>
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? > <http://fate-suite.ffmpeg.org/h264/reinit-large_420_8-to- > 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
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 [...]
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. regards jens
From b724bbe3a13addb055da883cbe802eee74d7c65e Mon Sep 17 00:00:00 2001 From: Jens Ziller <zillevdr@gmx.de> 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], &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); + } + 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