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

[Jorge Ramirez-Ortiz]

On 10/04/2017 06:23 PM, Mark Thompson wrote:
> On 04/10/17 16:48, Jorge Ramirez-Ortiz wrote:
>> On 10/04/2017 11:28 AM, Jorge Ramirez-Ortiz wrote:
>>> On 10/04/2017 11:23 AM, wm4 wrote:
>>>> On Wed,  4 Oct 2017 11:17:24 +0200
>>>> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz at linaro.org> wrote:
>>>>
>>>>> Some V4L2 drivers fail to allocate buffers when sizeimage is not set
>>>>> to a max value. This is indeed the case for sp5-mfc [1]
>>>>>
>>>>> Most drivers should be able to calculate this value from the frame
>>>>> dimensions and format - or at least have their own default.
>>>>>
>>>>> However since this work around should not impact those drivers doing
>>>>> the "right thing" this commit just provides such a default.
>>>>>
>>>>> [1] linux.git/drivers/media/platform/s5p-mfc
>>>>> ---
>>>>>    libavcodec/v4l2_context.c | 14 ++++++++++++--
>>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> 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
> I think you are doing the same thing here as <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/vaapi_encode.c;h=e13e99587dff35b1aa72f8d50f8da03cf4ffbb6e;hb=HEAD#l1239>.  The fixed 2MB feels likely to be too low for high-bandwidth video - you only need 480Mbps at 30fps to make frames which are 2MB *on average*, with intra frames being larger.

ah ok. thanks for the info!

>
> The calculated numbers for Qualcomm are probably fine, but do consider the possible failure modes - what would happen if it were exceeded?  (For H.264 at least you might be able to invoke the level restriction on minimum-compression to say that doesn't happen, but I don't know about other cases.)

sure. that can only happen at runtime - not when buffers are being requested - 
so the v4l2 the driver would report the error back to ffmpeg; and this error 
(like any other error from the driver) is handled with the current framework so 
I believe we are safe.

>
> In any case, please add a comment to say where whatever number you use came from.

ok.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel