| Message ID | 1507108644-20466-1-git-send-email-jorge.ramirez-ortiz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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?
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
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
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.
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.
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.
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
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
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.
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.
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; + } } }