Opened 6 years ago

Last modified 6 years ago

#7686 new defect

Wrong decision of pixel format in filtergraph

Reported by: Muhammad Faiz Owned by:
Priority: normal Component: avfilter
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Summary of the bug:
How to reproduce:

% ffmpeg -loglevel verbose -filter_complex "allrgb=d=0.1, lutyuv, format=yuv444p12be" -f null -

[Parsed_allrgb_0 @ 0x1b29f40] size:4096x4096 rate:25/1 duration:0.100000 sar:1/1
[auto_scaler_0 @ 0x1bad300] w:iw h:ih flags:'bilinear' interl:0
[Parsed_lutyuv_1 @ 0x1b2ab40] auto-inserting filter 'auto_scaler_0' between the filter 'Parsed_allrgb_0' and the filter 'Parsed_lutyuv_1'
[auto_scaler_1 @ 0x1bae9c0] w:iw h:ih flags:'bilinear' interl:0
[Parsed_format_2 @ 0x1b2a840] auto-inserting filter 'auto_scaler_1' between the filter 'Parsed_lutyuv_1' and the filter 'Parsed_format_2'
[auto_scaler_0 @ 0x1bad300] w:4096 h:4096 fmt:rgb24 sar:1/1 -> w:4096 h:4096 fmt:yuv444p sar:1/1 flags:0x2
[auto_scaler_1 @ 0x1bae9c0] w:4096 h:4096 fmt:yuv444p sar:1/1 -> w:4096 h:4096 fmt:yuv444p12be sar:1/1 flags:0x2

Note that the conversion is: rgb24 => yuv444p => yuv444p12be.
I guess the correct conversion is: rgb24 => yuv444p12le => yuv444p12be (lutyuv supports yuv444p12le).

Change History (6)

comment:1 by Carl Eugen Hoyos, 6 years ago

Why do you think that another conversion is better? Is there a formal argumentation?

I am not convinced this can be fixed, there definitely are (similar) cases where you have to use the format filter to fix the conversion.

comment:2 by Muhammad Faiz, 6 years ago

A more severe example

% ffmpeg -loglevel verbose -filter_complex "aevalsrc=0:d=0.1, showcqt, format=bgr24" -f null -

[Parsed_showcqt_1 @ 0xe63ec0] auto-inserting filter 'auto_resampler_0' between the filter 'Parsed_aevalsrc_0' and the filter 'Parsed_showcqt_1'
[auto_scaler_0 @ 0xe67980] w:iw h:ih flags:'bilinear' interl:0
[Parsed_format_2 @ 0xe64f80] auto-inserting filter 'auto_scaler_0' between the filter 'Parsed_showcqt_1' and the filter 'Parsed_format_2'
[Parsed_aevalsrc_0 @ 0xe634c0] sample_rate:44100 chlayout:mono duration:100000
[auto_resampler_0 @ 0xe67b80] ch:1 chl:mono fmt:dblp r:44100Hz -> ch:2 chl:stereo fmt:flt r:44100Hz
[Parsed_showcqt_1 @ 0xe63ec0] video: 1920x1080 yuv420p 25/1 fps, bar_h = 524, axis_h = 32, sono_h = 524.
[Parsed_showcqt_1 @ 0xe63ec0] fft_len = 8192, cqt_len = 1920.
[Parsed_showcqt_1 @ 0xe63ec0] nb_cqt_coeffs = 44508.
[Parsed_showcqt_1 @ 0xe63ec0] audio: 44100 Hz, step = 294.
[auto_scaler_0 @ 0xe67980] w:1920 h:1080 fmt:yuv420p sar:1/1 -> w:1920 h:1080 fmt:bgr24 sar:1/1 flags:0x2

While showcqt actually supports rgb24, the filtergraph chooses yuv420p -> bgr24 rather than rgb24 -> bgr24. This is a problem because supported pixel/sample format is currently undocumented in filters' documentation. If it is documented, a user for example can carefully insert format=rgb24 before format=bgr24.

comment:3 by Carl Eugen Hoyos, 6 years ago

(Why don't you try to answer my questions?)

The second example looks like something that could be improved (the system tries to find a best pix_fmt between aevalsrc and showcqt if showcqt and format do not have a common pix_fmt, this seems not ideal since aevalsrc has no pix_fmt) but it is unrelated to your first example.
In any case, I wonder why your past tickets (that were fixed) were valid and showed all necessary information while you decided that this ticket does not deserve to show all necessary information.

comment:4 by Muhammad Faiz, 6 years ago

OK,
for lutyuv:
actual: rgb24 -> yuv444p -> yuv444p12be
ideal: rgb24 -> yuv444p12le -> yuv444p12be
The ideal pixfmt should be yuv444p12le because it is exactly yuv444p12be with different endianness. (OK, one can argue that yuv444p is closer to rgb24).

for showcqt:
actual: none -> yuv420p -> bgr24
ideal: none -> rgb24 -> bgr24
The ideal pixfmt should be rgb24 because it is exactly bgr24 with different red-green-blue ordering. And no one can argue that yuv420p is closer to 'none'.

The root cause as you stated is similar. The pixfmt decision on filter is solely based on input filters when there are no common pixfmts.

I posted lutyuv example because I was retrying to fix https://ffmpeg.org/pipermail/ffmpeg-devel/2016-June/195941.html

in reply to:  4 comment:5 by Carl Eugen Hoyos, 6 years ago

Replying to mfcc64:

The root cause as you stated is similar.

I don't think they are similar: The first case seems a matter of taste, I don't think you were able to explain formally why one is better, the second case is a question of knowing what comes further down the filter-chain. The second case cannot be fixed in general but the specific issue of a filter not providing any sample format and the later ones not having a format in common but very similar ones maybe fixable.

I posted lutyuv example because I was retrying to fix https://ffmpeg.org/pipermail/ffmpeg-devel/2016-June/195941.html

I believe you have to fix this in the fate subsystem, your patch seems to make a lot of sense (apart from the fact that you want to improve big-endian systems while being unable to test them).

comment:6 by Muhammad Faiz, 6 years ago

After investigating fate-run.sh, on pixfmts() actually the tested filter is guarded by format filter, so I try this:

% ffmpeg -loglevel verbose -filter_complex "allrgb=d=0.1, format=yuv444p12be, lutyuv, format=yuv444p12be" -f null -

[Parsed_allrgb_0 @ 0x1e84440] size:4096x4096 rate:25/1 duration:0.100000 sar:1/1
[auto_scaler_0 @ 0x1f080c0] w:iw h:ih flags:'bilinear' interl:0
[Parsed_format_1 @ 0x1e850c0] auto-inserting filter 'auto_scaler_0' between the filter 'Parsed_allrgb_0' and the filter 'Parsed_format_1'
[auto_scaler_1 @ 0x1f096c0] w:iw h:ih flags:'bilinear' interl:0
[Parsed_lutyuv_2 @ 0x1e85b00] auto-inserting filter 'auto_scaler_1' between the filter 'Parsed_format_1' and the filter 'Parsed_lutyuv_2'
[auto_scaler_2 @ 0x1f0ad00] w:iw h:ih flags:'bilinear' interl:0
[Parsed_format_3 @ 0x1e7fb40] auto-inserting filter 'auto_scaler_2' between the filter 'Parsed_lutyuv_2' and the filter 'Parsed_format_3'
[auto_scaler_0 @ 0x1f080c0] w:4096 h:4096 fmt:rgb24 sar:1/1 -> w:4096 h:4096 fmt:yuv444p12be sar:1/1 flags:0x2
[auto_scaler_1 @ 0x1f096c0] w:4096 h:4096 fmt:yuv444p12be sar:1/1 -> w:4096 h:4096 fmt:yuv444p16le sar:1/1 flags:0x2
[auto_scaler_2 @ 0x1f0ad00] w:4096 h:4096 fmt:yuv444p16le sar:1/1 -> w:4096 h:4096 fmt:yuv444p12be sar:1/1 flags:0x2

The result is unexpected: yuv444p12be -> yuv444p16le -> yuv444p12be.
The ideal should be: yuv444p12be -> yuv444p12le -> yuv444p12be.

Note: See TracTickets for help on using tickets.