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

[wm4]

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.

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