diff mbox series

[FFmpeg-devel] ffmpeg_filter: don't try to autorotate frames with hwaccel pixel formats

Message ID 20210921123254.1712-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] ffmpeg_filter: don't try to autorotate frames with hwaccel pixel formats
Related show

Checks

Context Check Description
andriy/configurex86 warning Failed to apply patch
andriy/configureppc warning Failed to apply patch

Commit Message

James Almer Sept. 21, 2021, 12:32 p.m. UTC
The transpose, rotate, hflip, and vflip filters don't support them.
Fixes ticket #9432.

Signed-off-by: James Almer <jamrial@gmail.com>
---
I'm surprised nobody tried to decode an mp4 video like those coming from a
cellphone camera using a hwaccel decoder. This would have been noticed much
earlier.

 fftools/ffmpeg_filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Eoff, Ullysses A Sept. 21, 2021, 1:05 p.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Tuesday, September 21, 2021 5:33 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] ffmpeg_filter: don't try to autorotate frames with hwaccel pixel formats
> 
> The transpose, rotate, hflip, and vflip filters don't support them.
> Fixes ticket #9432.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I'm surprised nobody tried to decode an mp4 video like those coming from a
> cellphone camera using a hwaccel decoder. This would have been noticed much
> earlier.
> 
>  fftools/ffmpeg_filter.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index da0d4faf54..cc3dc33246 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -699,6 +699,7 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>  {
>      AVFilterContext *last_filter;
>      const AVFilter *buffer_filt = avfilter_get_by_name("buffer");
> +    const AVPixFmtDescriptor *desc;
>      InputStream *ist = ifilter->ist;
>      InputFile     *f = input_files[ist->file_index];
>      AVRational tb = ist->framerate.num ? av_inv_q(ist->framerate) :
> @@ -756,7 +757,9 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>      av_freep(&par);
>      last_filter = ifilter->filter;
> 
> -    if (ist->autorotate) {
> +    desc = av_pix_fmt_desc_get(ifilter->format);
> +    // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
> +    if (ist->autorotate && desc && !(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
>          int32_t *displaymatrix = ifilter->displaymatrix;
>          double theta;
> 
> --
> 2.33.0
> 

LGTM and verified WFM with vaapi hwaccel.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Sept. 21, 2021, 1:07 p.m. UTC | #2
James Almer (12021-09-21):
> The transpose, rotate, hflip, and vflip filters don't support them.
> Fixes ticket #9432.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I'm surprised nobody tried to decode an mp4 video like those coming from a
> cellphone camera using a hwaccel decoder. This would have been noticed much
> earlier.
> 
>  fftools/ffmpeg_filter.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index da0d4faf54..cc3dc33246 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -699,6 +699,7 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>  {
>      AVFilterContext *last_filter;
>      const AVFilter *buffer_filt = avfilter_get_by_name("buffer");
> +    const AVPixFmtDescriptor *desc;
>      InputStream *ist = ifilter->ist;
>      InputFile     *f = input_files[ist->file_index];
>      AVRational tb = ist->framerate.num ? av_inv_q(ist->framerate) :
> @@ -756,7 +757,9 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>      av_freep(&par);
>      last_filter = ifilter->filter;
>  
> -    if (ist->autorotate) {

> +    desc = av_pix_fmt_desc_get(ifilter->format);
> +    // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
> +    if (ist->autorotate && desc && !(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {

I am rather unsure about the "&& desc". buffersink requires a valid
pixel format. If the code arrives there without one, either there is a
bug somewhere or there should be a clearer failure earlier.

So I think "av_assert0(desc);" would be more adequate.

>          int32_t *displaymatrix = ifilter->displaymatrix;
>          double theta;
>  

Regards,
James Almer Sept. 21, 2021, 1:39 p.m. UTC | #3
On 9/21/2021 10:07 AM, Nicolas George wrote:
> James Almer (12021-09-21):
>> The transpose, rotate, hflip, and vflip filters don't support them.
>> Fixes ticket #9432.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> I'm surprised nobody tried to decode an mp4 video like those coming from a
>> cellphone camera using a hwaccel decoder. This would have been noticed much
>> earlier.
>>
>>   fftools/ffmpeg_filter.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
>> index da0d4faf54..cc3dc33246 100644
>> --- a/fftools/ffmpeg_filter.c
>> +++ b/fftools/ffmpeg_filter.c
>> @@ -699,6 +699,7 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>>   {
>>       AVFilterContext *last_filter;
>>       const AVFilter *buffer_filt = avfilter_get_by_name("buffer");
>> +    const AVPixFmtDescriptor *desc;
>>       InputStream *ist = ifilter->ist;
>>       InputFile     *f = input_files[ist->file_index];
>>       AVRational tb = ist->framerate.num ? av_inv_q(ist->framerate) :
>> @@ -756,7 +757,9 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>>       av_freep(&par);
>>       last_filter = ifilter->filter;
>>   
>> -    if (ist->autorotate) {
> 
>> +    desc = av_pix_fmt_desc_get(ifilter->format);
>> +    // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
>> +    if (ist->autorotate && desc && !(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
> 
> I am rather unsure about the "&& desc". buffersink requires a valid
> pixel format. If the code arrives there without one, either there is a
> bug somewhere or there should be a clearer failure earlier.
> 
> So I think "av_assert0(desc);" would be more adequate.

Will apply with that change. Thanks.

> 
>>           int32_t *displaymatrix = ifilter->displaymatrix;
>>           double theta;
>>   
> 
> Regards,
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
zhilizhao(赵志立) Sept. 21, 2021, 2:45 p.m. UTC | #4
> On Sep 21, 2021, at 8:32 PM, James Almer <jamrial@gmail.com> wrote:
> 
> The transpose, rotate, hflip, and vflip filters don't support them.
> Fixes ticket #9432.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I'm surprised nobody tried to decode an mp4 video like those coming from a
> cellphone camera using a hwaccel decoder. This would have been noticed much
> earlier.

Actually before cf1746d7 "ffmpeg_videotoolbox: skip memory copy if 
hwaccel_output_format match”, autorotate works for videotoolbox in an
inefficiency way. I thought it was by design.

> 
> fftools/ffmpeg_filter.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index da0d4faf54..cc3dc33246 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -699,6 +699,7 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
> {
>     AVFilterContext *last_filter;
>     const AVFilter *buffer_filt = avfilter_get_by_name("buffer");
> +    const AVPixFmtDescriptor *desc;
>     InputStream *ist = ifilter->ist;
>     InputFile     *f = input_files[ist->file_index];
>     AVRational tb = ist->framerate.num ? av_inv_q(ist->framerate) :
> @@ -756,7 +757,9 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
>     av_freep(&par);
>     last_filter = ifilter->filter;
> 
> -    if (ist->autorotate) {
> +    desc = av_pix_fmt_desc_get(ifilter->format);
> +    // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
> +    if (ist->autorotate && desc && !(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
>         int32_t *displaymatrix = ifilter->displaymatrix;
>         double theta;
> 
> -- 
> 2.33.0
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Sept. 21, 2021, 7:27 p.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Tuesday, 21 September 2021 14:33
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] ffmpeg_filter: don't try to autorotate frames
> with hwaccel pixel formats
> 
> The transpose, rotate, hflip, and vflip filters don't support them.
> Fixes ticket #9432.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> I'm surprised nobody tried to decode an mp4 video like those coming from a
> cellphone camera using a hwaccel decoder. This would have been noticed much
> earlier.

I'm aware of this issue, but I think it would be better to auto-insert the
corresponding hw rotation filters, as a user should be able to expect the
same result, no matter whether using sw or hw filtering.

It is already possible to specify 

-autorotate 0

to prevent autorotation, which applies to all cases equally.

softworkz
diff mbox series

Patch

diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index da0d4faf54..cc3dc33246 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -699,6 +699,7 @@  static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
 {
     AVFilterContext *last_filter;
     const AVFilter *buffer_filt = avfilter_get_by_name("buffer");
+    const AVPixFmtDescriptor *desc;
     InputStream *ist = ifilter->ist;
     InputFile     *f = input_files[ist->file_index];
     AVRational tb = ist->framerate.num ? av_inv_q(ist->framerate) :
@@ -756,7 +757,9 @@  static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
     av_freep(&par);
     last_filter = ifilter->filter;
 
-    if (ist->autorotate) {
+    desc = av_pix_fmt_desc_get(ifilter->format);
+    // TODO: insert hwaccel enabled filters like transpose_vaapi into the graph
+    if (ist->autorotate && desc && !(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
         int32_t *displaymatrix = ifilter->displaymatrix;
         double theta;