[FFmpeg-devel] [PATCH] avcodec/v4l2: set sizeimage param for non-raw buffers [fixes #6716]

[wm4]

On Wed, 4 Oct 2017 18:13:24 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz at linaro.org> wrote:

> On 10/04/2017 05:59 PM, wm4 wrote:
> >>>>> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
> >>>>> index 297792f..2707ef5 100644
> >>>>> --- a/libavcodec/v4l2_context.c
> >>>>> +++ b/libavcodec/v4l2_context.c
> >>>>> @@ -92,6 +92,8 @@ static inline int v4l2_type_supported(V4L2Context *ctx)
> >>>>>      static inline void v4l2_save_to_context(V4L2Context* ctx, struct
> >>>>> v4l2_format_update *fmt)
> >>>>>    {
> >>>>> +    const int MAX_SIZEIMAGE = 2 * 1024 * 1024;
> >>>>> +
> >>>>>        ctx->format.type = ctx->type;
> >>>>>          if (fmt->update_avfmt)
> >>>>> @@ -101,13 +103,21 @@ static inline void v4l2_save_to_context(V4L2Context*
> >>>>> ctx, struct v4l2_format_upd
> >>>>>            /* update the sizes to handle the reconfiguration of the capture
> >>>>> stream at runtime */
> >>>>>            ctx->format.fmt.pix_mp.height = ctx->height;
> >>>>>            ctx->format.fmt.pix_mp.width = ctx->width;
> >>>>> -        if (fmt->update_v4l2)
> >>>>> +        if (fmt->update_v4l2) {
> >>>>>                ctx->format.fmt.pix_mp.pixelformat = fmt->v4l2_fmt;
> >>>>> +
> >>>>> +            /* s5p-mfc requires the user to specify MAX buffer size */
> >>>>> +            ctx->format.fmt.pix_mp.plane_fmt[0].sizeimage = MAX_SIZEIMAGE;
> >>>>> +        }
> >>>>>        } else {
> >>>>>            ctx->format.fmt.pix.height = ctx->height;
> >>>>>            ctx->format.fmt.pix.width = ctx->width;
> >>>>> -        if (fmt->update_v4l2)
> >>>>> +        if (fmt->update_v4l2) {
> >>>>>                ctx->format.fmt.pix.pixelformat = fmt->v4l2_fmt;
> >>>>> +
> >>>>> +            /* s5p-mfc requires the user to specify MAX buffer size */
> >>>>> +            ctx->format.fmt.pix.sizeimage = MAX_SIZEIMAGE;
> >>>>> +        }
> >>>>>        }
> >>>>>    }  
> >>>> Isn't this something that should be fixed in the driver?  
> >>> yes but it might take forever and I dont know how many other drivers might
> >>> need it.
> >>>     
> >>>> Why 2MB?  
> >>> no analysis done but seems to be enough to hold an encoded frame. Should it be
> >>> any bigger?  
> >> I could use the calculations below if a generic magic number is a problem:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/qcom/venus/venc.c#n52
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/qcom/venus/vdec.c#n49
> >>
> >> please let me know  
> > Well, I don't think there's any reason why the frame size would be
> > limited to 2MB. I also can't tell if this is for uncompressed or
> > compressed frames. For uncompressed frames, you could easily compute a
> > good guess (the exact size depends on alignment and padding). For
> > compressed frames it's probably impossible.  
> 
> yes this is for compressed frames
> 
> >
> > If the kernel driver somehow can't be fixed and if this is a show
> > stopper, it's probably OK if this is done to unbreak it, but it should  
> 
> I doubt the kernel driver will be fixed any time soon - I can try posting a 
> patch there.
> 
> But even then if it gets merged people using older kernels will have to back 
> port to their kernels and it ends up being a pain for everyone. Since in this 
> case userspace can easily take care of it - is a minor change- I think it should 
> be merged in ffmpeg.

So would it break for better drivers if a packet of over 2 MB is fed to
them?

If yes, you'd have to detect the driver and explicitly use the
workaround for the buggy driver only.

> I pull the calculation from venc.c and vdec.c instead of the magic number.

To be honest this heuristic isn't better than the fixed constant
anyway. Both can break, except that the heuristic is more complex.