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

Submitted by Jorge Ramirez-Ortiz on Oct. 4, 2017, 9:17 a.m.

Details

Message ID 1507108644-20466-1-git-send-email-jorge.ramirez-ortiz@linaro.org
State New
Headers show

Commit Message

Jorge Ramirez-Ortiz Oct. 4, 2017, 9:17 a.m.
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(-)

Comments

wm4 Oct. 4, 2017, 9:23 a.m.
On Wed,  4 Oct 2017 11:17:24 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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?

Why 2MB?
Jorge Ramirez-Ortiz Oct. 4, 2017, 9:28 a.m.
On 10/04/2017 11:23 AM, wm4 wrote:
> On Wed,  4 Oct 2017 11:17:24 +0200
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Jorge Ramirez-Ortiz Oct. 4, 2017, 3:48 p.m.
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@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
wm4 Oct. 4, 2017, 3:59 p.m.
On Wed, 4 Oct 2017 17:48:25 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> 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@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

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.

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
at least not break other drivers/files which go beyond the limit.
Jorge Ramirez-Ortiz Oct. 4, 2017, 4:13 p.m.
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.

I pull the calculation from venc.c and vdec.c instead of the magic number.
wm4 Oct. 4, 2017, 4:20 p.m.
On Wed, 4 Oct 2017 18:13:24 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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.
Mark Thompson Oct. 4, 2017, 4:23 p.m.
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@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.

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

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

- Mark
Jorge Ramirez-Ortiz Oct. 4, 2017, 4:48 p.m.
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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Jorge Ramirez-Ortiz Oct. 4, 2017, 5:03 p.m.
On 10/04/2017 06:20 PM, wm4 wrote:
>>>>>> 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?

any good driver should encapsulate its own restrictions and not export them to 
the client as it is the case on s5p-mfc - so drivers properly written will 
ignore the sizeimage field.
This commit is just a workaround for a particular driver (since this driver was 
upstreamed more than 5 years ago it is hard to say if what they did followed the 
API at the time).

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

no, it will not affect properly - recently? - written drivers which should use 
the frame dimensions to allocate the compressed buffers.

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

um maybe- different hardware might have different alignment restrictions - But 
since the calculation seemed to roughly match Mark Thompson's in VAAPI I suppose 
it is good enough.
wm4 Oct. 4, 2017, 6:49 p.m.
On Wed, 4 Oct 2017 19:03:12 +0200
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:

> On 10/04/2017 06:20 PM, wm4 wrote:
> >>>>>> 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?  
> 
> any good driver should encapsulate its own restrictions and not export them to 
> the client as it is the case on s5p-mfc - so drivers properly written will 
> ignore the sizeimage field.

Sounds good then.

Patch hide | download patch | download mbox

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;
+        }
     }
 }