[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 <michael at niedermayer.cc>
>> Reply-to: FFmpeg development discussions and patches <ffmpeg-
>> devel at ffmpeg.org>
>> An: FFmpeg development discussions and patches <ffmpeg-devel at 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