[FFmpeg-devel] [PATCH] mmaldec: Set the output pix_fmt after detecting format

[Julian Scheel]

On 21.10.15 19:11, wm4 wrote:
> On Wed, 21 Oct 2015 18:48:42 +0200
> Julian Scheel <julian at jusst.de> wrote:
>
>> Am 21.10.2015 um 17:24 schrieb wm4:
>>> On Wed, 21 Oct 2015 17:15:14 +0200
>>> Julian Scheel <julian at jusst.de> wrote:
>>>
>>>> Am 21.10.2015 um 16:18 schrieb wm4:
>>>>> On Wed, 21 Oct 2015 16:07:08 +0200
>>>>> Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>>>>>
>>>>>> On Wed, Oct 21, 2015 at 3:55 PM, Julian Scheel <julian at jusst.de> wrote:
>>>>>>> Wait for the first decoded frame to be returned by mmal before setting
>>>>>>> pix_fmt. This is important for avformat probing to work properly as it is one
>>>>>>> of the criterions to decide whether to decode a frame or not for probing.
>>>>>>>
>>>>>>> Signed-off-by: Julian Scheel <julian at jusst.de>
>>>>>>> ---
>>>>>>>     libavcodec/mmaldec.c | 10 +++++-----
>>>>>>>     1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
>>>>>>> index 7db90d2..429990a 100644
>>>>>>> --- a/libavcodec/mmaldec.c
>>>>>>> +++ b/libavcodec/mmaldec.c
>>>>>>> @@ -338,11 +338,6 @@ static av_cold int ffmmal_init_decoder(AVCodecContext *avctx)
>>>>>>>             return AVERROR(ENOSYS);
>>>>>>>         }
>>>>>>>
>>>>>>> -    if ((ret = ff_get_format(avctx, avctx->codec->pix_fmts)) < 0)
>>>>>>> -        return ret;
>>>>>>> -
>>>>>>> -    avctx->pix_fmt = ret;
>>>>>>> -
>>>>>>>         if ((status = mmal_component_create(MMAL_COMPONENT_DEFAULT_VIDEO_DECODER, &ctx->decoder)))
>>>>>>>             goto fail;
>>>>>>>
>>>>>>> @@ -678,6 +673,11 @@ static int ffmmal_read_frame(AVCodecContext *avctx, AVFrame *frame, int *got_fra
>>>>>>>
>>>>>>>                 av_log(avctx, AV_LOG_INFO, "Changing output format.\n");
>>>>>>>
>>>>>>> +            if ((ret = ff_get_format(avctx, avctx->codec->pix_fmts)) < 0)
>>>>>>> +                return ret;
>>>>>>> +
>>>>>>> +            avctx->pix_fmt = ret;
>>>>>>> +
>>>>>>>                 if ((status = mmal_port_disable(decoder->output[0])))
>>>>>>>                     goto done;
>>>>>>>
>>>>>>
>>>>>> pix_fmt is already used by the decoder before this point to decide if
>>>>>> mmal surfaces or memory buffers are to be used, changing it afterwards
>>>>>> will not have the same effect as doing it in init.
>>>>>
>>>>> Oh, you're right. It's used in ffmal_update_format(), so this patch can
>>>>> only work if the decoder sends MMAL_EVENT_FORMAT_CHANGED on init, or if
>>>>> the entire decoding init is delayed and not done in AVCodec.init.
>>>>
>>>> I think it would be sufficient to query the pix_fmt via ff_get_format in
>>>> ffmal_update_format instead of using avctx->pix_fmt. The other paramters
>>>> should be changeable without disabling the port. But this should be
>>>> tested probably :)
>>>> Regarding the question if MMAL_EVENT_FORMAT_CHANGED will be sent by the
>>>> decoder in any case I have opened an issue for clarification:
>>>> https://github.com/raspberrypi/firmware/issues/489
>>>
>>> ffmal_update_format() is also called in the init function. Isn't this what
>>> you're trying to avoid?
>>
>> Well yes, probably we could just skip that call. Can you possibly test
>> if that works without breaking your opaque use case?
>
> It's needed for decoder init.

Right, a part of it is needed.

> But I'm pretty sure it'd work for me if decoder init were delayed until
> the first AVCodec.decode/ffmmal_decode call. (But I'd resist if the
> first init were to happen any time later - makes it harder to fallback
> to software decoding if mmal doesn't work.)

That should be ok. I should be able to test that tomorrow.