| Message ID | 20171129011437.4125-1-neil.birkbeck@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 28, 2017 at 5:14 PM, Neil Birkbeck <neil.birkbeck@gmail.com> wrote: > - //The srcBpc check is possibly wrong but we seem to lack a definitive > reference to test this > - //and what we have in ticket 2939 looks better with this check > - if (need_reinit && (c->srcBpc == 8 || !isYUV(c->srcFormat))) > + if (need_reinit) > ff_sws_init_range_convert(c); > Wanted to check and see if someone more familiar with this condition can comment as to whether some parts of it may still be necessary. Also, this may also need to be coupled with a change to vf_scale. E.g., at the moment, I think scaling >10bit YUV without explicit _range settings may not change color range of the data (but it may assign incorrect color_range flag to the frame).
On Tue, Nov 28, 2017 at 05:14:37PM -0800, Neil Birkbeck wrote: > On higher bit depth YUV inputs with range metadata signaled as PC/full levels, > this change makes the results of scaling without explicitly > setting the input range on vf_scale as if it were explicitly set. > > For example, the results with implicit color settings from the frame: > -vf scale=-1:-1:out_range=mpeg,format=yuv420p > > Are the same as the correct result when set explicitly in the scaler: > -vf scale=-1:-1:in_range=jpeg:out_range=mpeg,format=yuv420p > > The results are consistent with a similar yuv420p(pc) test input > (e.g., implicit and explicit setting of in_range on vf_scale both work). > > Fate passes without the checks (or with a more specific check for >= 8). > If this seems sane, I'll write some tests. > > I tried to reproduce the old results from before and after the commit > that I think the previous comment was referring to > 4959a4fcf76e7c595dbb23c4e3bf59abf2e60ea4 > but failed to repro (I may be testing the wrong thing). Using the samples > from (https://trac.ffmpeg.org/ticket/2939), without the check: > ffmpeg -i /tmp/progressive.jpg -vf format=rgb24 /tmp/progressive.png > is still treated as full range input (treating it as studio causes clipping > in the rgb). If you are searching for a case where the patch makes a difference one is: ./ffmpeg -i ~/tickets/4493/AVCI100.mov out.nut file should be here: http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket524/ if you want more cases that change, ill see if i can find more [...]
> > > > If you are searching for a case where the patch makes a difference > one is: > ./ffmpeg -i ~/tickets/4493/AVCI100.mov out.nut > file should be here: > http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket524/ > > if you want more cases that change, ill see if i can find more > Perfect, thanks Michael. Let me check those samples out. Here is a comparison of what I was described in the commit log. The naming of the files images are ${pixfmt}_${scaled}_${in_range}_${out_range} for with/without the patch: https://rawgit.com/nbirkbeck/ffmpeg-test-samples/master/color-range/results/report.html My concern was the yuv444p10_${scaled}_jpeg_mpeg (explicit settings), give different results than the implicit yuv444p10_unscaled_unspec_mpeg ones for high bit depth. And the high bit depth is results are in general different than the low bit depth ones. Report generated with: https://raw.githubusercontent.com/nbirkbeck/ffmpeg-test-samples/master/color-range/run.sh Using these test files: https://github.com/nbirkbeck/ffmpeg-test-samples/tree/master/color-range/data
On Wed, Nov 29, 2017 at 7:40 PM, Neil Birkbeck <neil.birkbeck@gmail.com> wrote: > >> >> If you are searching for a case where the patch makes a difference >> one is: >> ./ffmpeg -i ~/tickets/4493/AVCI100.mov out.nut >> file should be here: >> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket524/ >> >> if you want more cases that change, ill see if i can find more >> > > Perfect, thanks Michael. Let me check those samples out. > > For that sample, I feel like it may be incorrectly tagged as pc/full. Looking at the histogram of the original there is no data in low range and a peak due to clipping near where you'd expect higher up for studio/mpeg range ffplay /tmp/AVCI100.mov -vf histogram And scaling, treating the input as studio/mpeg and outputting full range, stretches y to cover the entire range: ffplay /tmp/AVCI100.mov -vf scale=-1:-1:in_range=mpeg:out_range=jpeg,format=yuv422p10le,histogram This is a real concern though. I don't have a good feel for how many higher bit depth files are incorrectly labelled as pc/full. Here is a comparison of what I was described in the commit log. > > The naming of the files images are ${pixfmt}_${scaled}_${in_range}_${out_range} > for with/without the patch: > https://rawgit.com/nbirkbeck/ffmpeg-test-samples/master/ > color-range/results/report.html > > My concern was the yuv444p10_${scaled}_jpeg_mpeg (explicit settings), give > different results than the implicit yuv444p10_unscaled_unspec_mpeg ones for > high bit depth. And the high bit depth is results are in general different > than the low bit depth ones. > > Report generated with: > https://raw.githubusercontent.com/nbirkbeck/ffmpeg-test- > samples/master/color-range/run.sh > Using these test files: > https://github.com/nbirkbeck/ffmpeg-test-samples/tree/ > master/color-range/data >
On Wed, Nov 29, 2017 at 08:08:40PM -0800, Neil Birkbeck wrote: > On Wed, Nov 29, 2017 at 7:40 PM, Neil Birkbeck <neil.birkbeck@gmail.com> > wrote: > > > > >> > >> If you are searching for a case where the patch makes a difference > >> one is: > >> ./ffmpeg -i ~/tickets/4493/AVCI100.mov out.nut > >> file should be here: > >> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket524/ > >> > >> if you want more cases that change, ill see if i can find more > >> > > > > Perfect, thanks Michael. Let me check those samples out. there are 2 more in 2939 which change: https://trac.ffmpeg.org/ticket/2939 > > > > > For that sample, I feel like it may be incorrectly tagged as pc/full. is it stored in the file or taken from: ff_generate_avci_extradata() maybe theres a bug in the AVCIntra handling > Looking at the histogram of the original there is no data in low range and > a peak due to clipping near where you'd expect higher up for studio/mpeg > range > ffplay /tmp/AVCI100.mov -vf histogram > > And scaling, treating the input as studio/mpeg and outputting full range, > stretches y to cover the entire range: > ffplay /tmp/AVCI100.mov -vf > scale=-1:-1:in_range=mpeg:out_range=jpeg,format=yuv422p10le,histogram > > This is a real concern though. I don't have a good feel for how many higher > bit depth files are incorrectly labelled as pc/full. > > Here is a comparison of what I was described in the commit log. > > > > The naming of the files images are ${pixfmt}_${scaled}_${in_range}_${out_range} > > for with/without the patch: > > https://rawgit.com/nbirkbeck/ffmpeg-test-samples/master/ > > color-range/results/report.html > > > > My concern was the yuv444p10_${scaled}_jpeg_mpeg (explicit settings), give > > different results than the implicit yuv444p10_unscaled_unspec_mpeg ones for > > high bit depth. And the high bit depth is results are in general different > > than the low bit depth ones. > > > > Report generated with: > > https://raw.githubusercontent.com/nbirkbeck/ffmpeg-test- > > samples/master/color-range/run.sh Can you turn this into a fate test ? > > Using these test files: > > https://github.com/nbirkbeck/ffmpeg-test-samples/tree/ > > master/color-range/data > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Nov 30, 2017 at 9:52 AM, Michael Niedermayer <michael@niedermayer.cc > wrote: > > > Perfect, thanks Michael. Let me check those samples out. > > there are 2 more in 2939 which change: > https://trac.ffmpeg.org/ticket/2939 > It seems the swscale_unscaled code paths do not get reconfigured when sws_setColorSpaceDetails is called. The unscaled code path also seems to get called for yuv420p16le->yuv420p, so the patch only affected those samples when doing some scaling. But the newer results looks consistent with direct conversion to rgb (and is consistent when I decode the samples in matlab). > > > > > For that sample, I feel like it may be incorrectly tagged as pc/full. > > is it stored in the file or taken from: > ff_generate_avci_extradata() > maybe theres a bug in the AVCIntra handling > > It seems avci100_1080i_extradata may be the one that is signalling full range for the AVCI100.mov sample. I tested changing the range flag: - 0x3c, 0x60, 0x20, 0x20, 0x28, 0x00, 0x00, 0x03, + 0x3c, 0x20, 0x20, 0x20, 0x28, 0x00, 0x00, 0x03, There is an unused ACLR atom in the mov that also appears to signal full range (parsing of that atom is skipped for h264) > > > > Report generated with: > > > https://raw.githubusercontent.com/nbirkbeck/ffmpeg-test- > > > samples/master/color-range/run.sh > > Can you turn this into a fate test ? > Will do.
2017-12-01 20:08 GMT+01:00 Neil Birkbeck <neil.birkbeck@gmail.com>: > On Thu, Nov 30, 2017 at 9:52 AM, Michael Niedermayer wrote: >> > For that sample, I feel like it may be incorrectly tagged as pc/full. >> >> is it stored in the file or taken from: >> ff_generate_avci_extradata() >> maybe theres a bug in the AVCIntra handling > > It seems avci100_1080i_extradata may be the one that is signalling full > range for the AVCI100.mov sample. I tested changing the range flag: > - 0x3c, 0x60, 0x20, 0x20, 0x28, 0x00, 0x00, 0x03, > + 0x3c, 0x20, 0x20, 0x20, 0x28, 0x00, 0x00, 0x03, If you believe it is safe (or a bug), please change it. > There is an unused ACLR atom in the mov that also appears to > signal full range (parsing of that atom is skipped for h264) I believe the extra-data should not be overwritten. Carl Eugen
diff --git a/libswscale/utils.c b/libswscale/utils.c index 4df09306d3..f900e3bdef 100644 --- a/libswscale/utils.c +++ b/libswscale/utils.c @@ -883,9 +883,7 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], c->srcRange = srcRange; c->dstRange = dstRange; - //The srcBpc check is possibly wrong but we seem to lack a definitive reference to test this - //and what we have in ticket 2939 looks better with this check - if (need_reinit && (c->srcBpc == 8 || !isYUV(c->srcFormat))) + if (need_reinit) ff_sws_init_range_convert(c); c->dstFormatBpp = av_get_bits_per_pixel(desc_dst);
On higher bit depth YUV inputs with range metadata signaled as PC/full levels, this change makes the results of scaling without explicitly setting the input range on vf_scale as if it were explicitly set. For example, the results with implicit color settings from the frame: -vf scale=-1:-1:out_range=mpeg,format=yuv420p Are the same as the correct result when set explicitly in the scaler: -vf scale=-1:-1:in_range=jpeg:out_range=mpeg,format=yuv420p The results are consistent with a similar yuv420p(pc) test input (e.g., implicit and explicit setting of in_range on vf_scale both work). Fate passes without the checks (or with a more specific check for >= 8). If this seems sane, I'll write some tests. I tried to reproduce the old results from before and after the commit that I think the previous comment was referring to 4959a4fcf76e7c595dbb23c4e3bf59abf2e60ea4 but failed to repro (I may be testing the wrong thing). Using the samples from (https://trac.ffmpeg.org/ticket/2939), without the check: ffmpeg -i /tmp/progressive.jpg -vf format=rgb24 /tmp/progressive.png is still treated as full range input (treating it as studio causes clipping in the rgb). There are still some other edge cases where range conversion doesn't work unless explicitly set (e.g., when no scale is happening) Signed-off-by: Neil Birkbeck <neil.birkbeck@gmail.com> --- libswscale/utils.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)