[FFmpeg-devel,RFC] lavc/ffv1dec: Scale msb-packed output to full 16bit

Submitted by Carl Eugen Hoyos on Nov. 16, 2016, 11:15 a.m.

Details

Message ID 201611161215.28889.cehoyos@ag.or.at
State New
Headers show

Commit Message

Carl Eugen Hoyos Nov. 16, 2016, 11:15 a.m.
Hi!

Attached patch improves output for some ffv1 files imo.
Current slowdown for the existing decode-line timer is 
2%, I wonder if this can be improved through refactoring.

Please comment, Carl Eugen
From e4e8277d4f0daeb9f61c2da9ebb167a084b1b93c Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Wed, 16 Nov 2016 12:03:17 +0100
Subject: [PATCH] lavc/ffv1dec: Scale output for msb-packed compression to
 full 16bit.

2% slowdown for existing decode-line timer.
---
 libavcodec/ffv1dec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Hendrik Leppkes Nov. 16, 2016, 11:38 a.m.
On Wed, Nov 16, 2016 at 12:15 PM, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> Hi!
>
> Attached patch improves output for some ffv1 files imo.
> Current slowdown for the existing decode-line timer is
> 2%, I wonder if this can be improved through refactoring.
>
> Please comment, Carl Eugen
>

Isn't ffv1 lossless? How can it improve? Was it not lossless before,
is it lossless now?

- Hendrik
Carl Eugen Hoyos Nov. 16, 2016, 11:50 a.m.
2016-11-16 12:38 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> On Wed, Nov 16, 2016 at 12:15 PM, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>> Hi!
>>
>> Attached patch improves output for some ffv1 files imo.
>> Current slowdown for the existing decode-line timer is
>> 2%, I wonder if this can be improved through refactoring.
>>
>> Please comment, Carl Eugen
>>
>
> Isn't ffv1 lossless? How can it improve? Was it not lossless before,
> is it lossless now?

It was lossless before and is lossless after the patch but it
output the 16bit format that you don't like and of which you
claim it breaks with find_best_pix_fmt() - I still believe this
should be trivial to fix.

Currently white is gray, the patch tries to fix this.

Carl Eugen
Michael Niedermayer Nov. 16, 2016, 2:02 p.m.
On Wed, Nov 16, 2016 at 12:15:28PM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch improves output for some ffv1 files imo.
> Current slowdown for the existing decode-line timer is 
> 2%, I wonder if this can be improved through refactoring.
> 
> Please comment, Carl Eugen

>  ffv1dec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> ab0f3ff58a18145efd66d627ecbf54c35b5c23e6  0001-lavc-ffv1dec-Scale-output-for-lsb-packed-compression.patch
> From e4e8277d4f0daeb9f61c2da9ebb167a084b1b93c Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> Date: Wed, 16 Nov 2016 12:03:17 +0100
> Subject: [PATCH] lavc/ffv1dec: Scale output for msb-packed compression to
>  full 16bit.
> 
> 2% slowdown for existing decode-line timer.
> ---
>  libavcodec/ffv1dec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

probably ok

also try if using a temporary variable  is faster


[...]
Kieran Kunhya Nov. 16, 2016, 11:49 p.m.
>
> It was lossless before and is lossless after the patch but it
> output the 16bit format that you don't like and of which you
> claim it breaks with find_best_pix_fmt() - I still believe this
> should be trivial to fix.
>
> Currently white is gray, the patch tries to fix this.
>

erm...in what way does it fix that?
A white/black value in 16-235 needs to remain like that when shifted. This
isn't RGB-land.

Kieran
Rostislav Pehlivanov Nov. 17, 2016, 1:49 p.m.
On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:

> Hi!
>
> Attached patch improves output for some ffv1 files imo.
> Current slowdown for the existing decode-line timer is
> 2%, I wonder if this can be improved through refactoring.
>
> Please comment, Carl Eugen
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
So AFAIK the encoder pushes the values to the LSBs but the decoder didn't
shift them back up?
I think you should add a comment explaining that happens. Also 2% on a
decoder doesn't sound
that great, did you try using an if case for the entire loop for when the
values need to be shifted?
Kieran Kunhya Nov. 17, 2016, 6:05 p.m.
On Wed, 16 Nov 2016 at 23:49 Kieran Kunhya <kierank@obe.tv> wrote:

> It was lossless before and is lossless after the patch but it
> output the 16bit format that you don't like and of which you
> claim it breaks with find_best_pix_fmt() - I still believe this
> should be trivial to fix.
>
> Currently white is gray, the patch tries to fix this.
>
>
> erm...in what way does it fix that?
> A white/black value in 16-235 needs to remain like that when shifted. This
> isn't RGB-land.
>

This incorrect patch has been committed in spite of clear objections:

https://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=55a424c5a836523828b3b2f02b7db610e898b3fc

Regards,
Kieran Kunhya
Kieran Kunhya Nov. 17, 2016, 6:06 p.m.
On Thu, 17 Nov 2016 at 18:05 Kieran Kunhya <kierank@obe.tv> wrote:

> On Wed, 16 Nov 2016 at 23:49 Kieran Kunhya <kierank@obe.tv> wrote:
>
> It was lossless before and is lossless after the patch but it
> output the 16bit format that you don't like and of which you
> claim it breaks with find_best_pix_fmt() - I still believe this
> should be trivial to fix.
>
> Currently white is gray, the patch tries to fix this.
>
>
> erm...in what way does it fix that?
> A white/black value in 16-235 needs to remain like that when shifted. This
> isn't RGB-land.
>
>
> This incorrect patch has been committed in spite of clear objections:
>
>
> https://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=55a424c5a836523828b3b2f02b7db610e898b3fc
>
> Regards,
> Kieran Kunhya
>

Furthermore I will be reverting this patch in two hours.

Kieran
Ronald S. Bultje Nov. 17, 2016, 6:34 p.m.
Hi,

On Thu, Nov 17, 2016 at 1:06 PM, Kieran Kunhya <kierank@obe.tv> wrote:

> On Thu, 17 Nov 2016 at 18:05 Kieran Kunhya <kierank@obe.tv> wrote:
>
> > On Wed, 16 Nov 2016 at 23:49 Kieran Kunhya <kierank@obe.tv> wrote:
> >
> > It was lossless before and is lossless after the patch but it
> > output the 16bit format that you don't like and of which you
> > claim it breaks with find_best_pix_fmt() - I still believe this
> > should be trivial to fix.
> >
> > Currently white is gray, the patch tries to fix this.
> >
> >
> > erm...in what way does it fix that?
> > A white/black value in 16-235 needs to remain like that when shifted.
> This
> > isn't RGB-land.
> >
> >
> > This incorrect patch has been committed in spite of clear objections:
> >
> >
> > https://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=
> 55a424c5a836523828b3b2f02b7db610e898b3fc
> >
> > Regards,
> > Kieran Kunhya
> >
>
> Furthermore I will be reverting this patch in two hours.


There's no need to wait two hours, the patch is fundamentally wrong.

Carl, the reason the patch is wrong is that luma range does not scale up
from 16<<n to (236<<n)-1, but from 16<<n to (236-1)<<n, where n is bpc-8.
This is documented in numerous places, see e.g.
https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx
:

"For example, if the white point of an 8-bit format is 235, the
corresponding 10-bit format has a white point at 940 (235 × 4)."

Your patch is therefore theoretically wrong. The correct behaviour in
limited-range upscaling is indeed to leave the lower bits empty. For
full-range, the lower bits might be filled if the intention is for the
pixel value to be a representation of what the 16bit result would look
like, but whether this belongs in a decoder or not is up for discussion.

Ronald
Carl Eugen Hoyos Nov. 17, 2016, 8:06 p.m.
2016-11-17 19:06 GMT+01:00 Kieran Kunhya <kierank@obe.tv>:
> On Thu, 17 Nov 2016 at 18:05 Kieran Kunhya <kierank@obe.tv> wrote:

> Furthermore I will be reverting this patch in two hours.

Please do not revert a patch reviewed by the maintainer.

Carl Eugen
Carl Eugen Hoyos Nov. 17, 2016, 8:10 p.m.
Hi!

2016-11-17 19:34 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
>
> On Thu, Nov 17, 2016 at 1:06 PM, Kieran Kunhya <kierank@obe.tv> wrote:

>> Furthermore I will be reverting this patch in two hours.
>
> There's no need to wait two hours, the patch is fundamentally wrong.

This is not a helpful comment.

> Carl, the reason the patch is wrong is that luma range does not scale up
> from 16<<n to (236<<n)-1, but from 16<<n to (236-1)<<n, where n is bpc-8.
> This is documented in numerous places, see e.g.
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx
> :
>
> "For example, if the white point of an 8-bit format is 235, the
> corresponding 10-bit format has a white point at 940 (235 × 4)."

This is indeed very difficult to understand: How is this related?

AV_PIX_FMT_GRAY was changed years ago after several users
protested that it did conform to above specification, since it doesn't
do now, the paragraph looks unrelated.
(AV_PIX_FMT_GRAY is full-scale as is AV_PIX_FMT_GRAY16)

More important:
My patch is not related to 10-bit output format, so above calculations
are also not related afaict.

> Your patch is therefore theoretically wrong. The correct behaviour in
> limited-range upscaling is indeed to leave the lower bits empty. For
> full-range, the lower bits might be filled if the intention is for the
> pixel value to be a representation of what the 16bit result would look
> like, but whether this belongs in a decoder or not is up for discussion.

Decoders tend to be used by video players and if white looks gray on a
screen, it doesn't make much sense to say "the player should have
worked-around this".

Carl Eugen
Carl Eugen Hoyos Nov. 17, 2016, 8:11 p.m.
2016-11-16 15:02 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
> On Wed, Nov 16, 2016 at 12:15:28PM +0100, Carl Eugen Hoyos wrote:
>> Hi!
>>
>> Attached patch improves output for some ffv1 files imo.
>> Current slowdown for the existing decode-line timer is
>> 2%, I wonder if this can be improved through refactoring.
>>
>> Please comment, Carl Eugen
>
>>  ffv1dec.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> ab0f3ff58a18145efd66d627ecbf54c35b5c23e6  0001-lavc-ffv1dec-Scale-output-for-lsb-packed-compression.patch
>> From e4e8277d4f0daeb9f61c2da9ebb167a084b1b93c Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Wed, 16 Nov 2016 12:03:17 +0100
>> Subject: [PATCH] lavc/ffv1dec: Scale output for msb-packed compression to
>>  full 16bit.
>>
>> 2% slowdown for existing decode-line timer.
>> ---
>>  libavcodec/ffv1dec.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> probably ok
>
> also try if using a temporary variable  is faster

Unfortunately not.

Patch applied, Carl Eugen
Carl Eugen Hoyos Nov. 17, 2016, 8:13 p.m.
2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov <atomnuker@gmail.com>:
> On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>
>> Hi!
>>
>> Attached patch improves output for some ffv1 files imo.
>> Current slowdown for the existing decode-line timer is
>> 2%, I wonder if this can be improved through refactoring.
>>
>> Please comment, Carl Eugen
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
> So AFAIK the encoder pushes the values to the LSBs but the decoder didn't
> shift them back up?

I don't think the encoder does any shifts here but I may misunderstand.

> I think you should add a comment explaining that happens.

Many (older) decoders have to do this and there is nowhere a
comment, I really believe that this is not particularly convoluted
code.

> Also 2% on a decoder doesn't sound that great,

It's 2% in a function of a decoder.

> did you try using an if case for the entire loop for when the
> values need to be shifted?

That is what I tried to suggest with "refactoring", I suspect
Michael wasn't too happy about the idea.

Carl Eugen
Carl Eugen Hoyos Nov. 17, 2016, 8:16 p.m.
2016-11-17 0:49 GMT+01:00 Kieran Kunhya <kierank@obe.tv>:
>>
>> It was lossless before and is lossless after the patch but it
>> output the 16bit format that you don't like and of which you
>> claim it breaks with find_best_pix_fmt() - I still believe this
>> should be trivial to fix.
>>
>> Currently white is gray, the patch tries to fix this.
>
> erm...in what way does it fix that?

White output stops looking gray.

> A white/black value in 16-235 needs to remain like that when shifted.

Since this patch isn't about 8bit (what 16-235 indicate),
I didn't know what to do with your comment, sorry.

>This isn't RGB-land.

As just written in the other mail:
AV_PIX_FMT_GRAY8 and AV_PIX_FMT_GRAY16 are full-scale
(because of repeated user-request).
If you want to change this, please suggest a patch but this patch
fixes current behaviour of one decoder compared to the scaler
and all other decoders.

Carl Eugen
wm4 Nov. 17, 2016, 8:32 p.m.
On Thu, 17 Nov 2016 21:06:33 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2016-11-17 19:06 GMT+01:00 Kieran Kunhya <kierank@obe.tv>:
> > On Thu, 17 Nov 2016 at 18:05 Kieran Kunhya <kierank@obe.tv> wrote:  
> 
> > Furthermore I will be reverting this patch in two hours.  
> 
> Please do not revert a patch reviewed by the maintainer.

"probably ok" is not much of a review. It also could mean "probably not
ok".
wm4 Nov. 17, 2016, 8:34 p.m.
On Thu, 17 Nov 2016 21:10:34 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Hi!
> 
> 2016-11-17 19:34 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> >
> > On Thu, Nov 17, 2016 at 1:06 PM, Kieran Kunhya <kierank@obe.tv> wrote:  
> 
> >> Furthermore I will be reverting this patch in two hours.  
> >
> > There's no need to wait two hours, the patch is fundamentally wrong.  
> 
> This is not a helpful comment.
> 
> > Carl, the reason the patch is wrong is that luma range does not scale up
> > from 16<<n to (236<<n)-1, but from 16<<n to (236-1)<<n, where n is bpc-8.
> > This is documented in numerous places, see e.g.
> > https://msdn.microsoft.com/en-us/library/windows/desktop/bb970578(v=vs.85).aspx
> > :
> >
> > "For example, if the white point of an 8-bit format is 235, the
> > corresponding 10-bit format has a white point at 940 (235 × 4)."  
> 
> This is indeed very difficult to understand: How is this related?

If you don't understand it, then why do you frivolously change the code?

> AV_PIX_FMT_GRAY was changed years ago after several users
> protested that it did conform to above specification, since it doesn't
> do now, the paragraph looks unrelated.
> (AV_PIX_FMT_GRAY is full-scale as is AV_PIX_FMT_GRAY16)

All YUV formats with more than 16 bit are "shifted", i.e. the lower
bits are always 0. Why should this be different for GRAY?

Unless you argue that GRAY is a RGB format.

> More important:
> My patch is not related to 10-bit output format, so above calculations
> are also not related afaict.
> 
> > Your patch is therefore theoretically wrong. The correct behaviour in
> > limited-range upscaling is indeed to leave the lower bits empty. For
> > full-range, the lower bits might be filled if the intention is for the
> > pixel value to be a representation of what the 16bit result would look
> > like, but whether this belongs in a decoder or not is up for discussion.  
> 
> Decoders tend to be used by video players and if white looks gray on a
> screen, it doesn't make much sense to say "the player should have
> worked-around this".

These video players are broken then. Or consider GRAY RGB.
James Almer Nov. 17, 2016, 8:34 p.m.
On 11/17/2016 5:11 PM, Carl Eugen Hoyos wrote:
> 2016-11-16 15:02 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>> On Wed, Nov 16, 2016 at 12:15:28PM +0100, Carl Eugen Hoyos wrote:
>>> Hi!
>>>
>>> Attached patch improves output for some ffv1 files imo.
>>> Current slowdown for the existing decode-line timer is
>>> 2%, I wonder if this can be improved through refactoring.
>>>
>>> Please comment, Carl Eugen
>>
>>>  ffv1dec.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> ab0f3ff58a18145efd66d627ecbf54c35b5c23e6  0001-lavc-ffv1dec-Scale-output-for-lsb-packed-compression.patch
>>> From e4e8277d4f0daeb9f61c2da9ebb167a084b1b93c Mon Sep 17 00:00:00 2001
>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>> Date: Wed, 16 Nov 2016 12:03:17 +0100
>>> Subject: [PATCH] lavc/ffv1dec: Scale output for msb-packed compression to
>>>  full 16bit.
>>>
>>> 2% slowdown for existing decode-line timer.
>>> ---
>>>  libavcodec/ffv1dec.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> probably ok
>>
>> also try if using a temporary variable  is faster
> 
> Unfortunately not.
> 
> Patch applied, Carl Eugen

You pushed it /before/ addressing people's objections and concerns.

A patch that changes the output of a decoder for a codec in the middle
of being standardized shouldn't be applied just like that. Especially
when it's a controversial change objected by several developers.
wm4 Nov. 17, 2016, 8:36 p.m.
On Thu, 17 Nov 2016 21:16:13 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2016-11-17 0:49 GMT+01:00 Kieran Kunhya <kierank@obe.tv>:
> >>
> >> It was lossless before and is lossless after the patch but it
> >> output the 16bit format that you don't like and of which you
> >> claim it breaks with find_best_pix_fmt() - I still believe this
> >> should be trivial to fix.
> >>
> >> Currently white is gray, the patch tries to fix this.  
> >
> > erm...in what way does it fix that?  
> 
> White output stops looking gray.

Stop trying to add bugs just because your trial&error seemed to have
shown an improvement.

This things don't "look" like anything - they have discrete output
values. kierank posted a useful link explaining how it works and why
the old code was correct - go read it.

> > A white/black value in 16-235 needs to remain like that when shifted.  
> 
> Since this patch isn't about 8bit (what 16-235 indicate),
> I didn't know what to do with your comment, sorry.
> 
> >This isn't RGB-land.  
> 
> As just written in the other mail:
> AV_PIX_FMT_GRAY8 and AV_PIX_FMT_GRAY16 are full-scale
> (because of repeated user-request).
> If you want to change this, please suggest a patch but this patch
> fixes current behaviour of one decoder compared to the scaler
> and all other decoders.

The GRAY formats are not marked as RGB formats.
Ronald S. Bultje Nov. 17, 2016, 8:53 p.m.
Hi Carl,

On Thu, Nov 17, 2016 at 3:10 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2016-11-17 19:34 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> > Carl, the reason the patch is wrong is that luma range does not scale up
> > from 16<<n to (236<<n)-1, but from 16<<n to (236-1)<<n, where n is bpc-8.
> > This is documented in numerous places, see e.g.
> > https://msdn.microsoft.com/en-us/library/windows/desktop/
> bb970578(v=vs.85).aspx
> > :
> >
> > "For example, if the white point of an 8-bit format is 235, the
> > corresponding 10-bit format has a white point at 940 (235 × 4)."
>
> This is indeed very difficult to understand: How is this related?
>
> AV_PIX_FMT_GRAY was changed years ago after several users
> protested that it did conform to above specification, since it doesn't
> do now, the paragraph looks unrelated.
> (AV_PIX_FMT_GRAY is full-scale as is AV_PIX_FMT_GRAY16)
>
> More important:
> My patch is not related to 10-bit output format, so above calculations
> are also not related afaict.


The relevant field in the decoder is called bits_per_raw_sample. If this is
10, the format is 10-bit. The output format is therefore also 10-bit,
otherwise it's not lossless. How you represent that in an uint16_t is up to
you, obviously, but it's 10bit, right? So the patch relates to 10bit
formats.

The question then seems, what do you do with the ("filler") bits if we
shift them to MSB in gray16, i.e. packed_at_lsb=0? My assumption (from the
decision to represent the coding as gray16 while setting
bits_per_raw_sample) is that you want it to represent a value in MSB of
uint16_t that would be closest to what it would have been if the value was
actually coded as 16bit, right?

(I'm assuming the above is logical and we all agree on it, please let me
know if you disagree, otherwise let's go on to the apparently controversial
stuff.)

> Your patch is therefore theoretically wrong. The correct behaviour in
> > limited-range upscaling is indeed to leave the lower bits empty. For
> > full-range, the lower bits might be filled if the intention is for the
> > pixel value to be a representation of what the 16bit result would look
> > like, but whether this belongs in a decoder or not is up for discussion.
>
> Decoders tend to be used by video players and if white looks gray on a
> screen, it doesn't make much sense to say "the player should have
> worked-around this".


I'm fine with that, but it depends on the format. If the input was
full-range, then yes. If the input was not full-range, then no.

Assume I have a H264 file with a PPS range=limited and a chroma_idc=0
(4:0:0, i.e. grayscale). You agree from the H264 spec that this is a legal
file, right? I could also generate source data by taking any (10bit) Bt709
YUV file (which almost universally use limited-range coding) and stripping
the UV planes out and leaving the Y data untouched.

How do I represent this data losslessly in ffv1 after your patch? I don't
think I can, because a value of 0x3ac (235x4=940, i.e. white) would be
upshifted to (0x3ac << 6) || (0x3ac >> 4) = 0xeb3a, which converting back
(as e.g. swscale does) to 10bits is 940.9, so after rounding it would be
941. That is not 940, thus not lossless and therefore a bug (IMO).

I agree that if the data was fullrange, your patch may be correct (it
depends on some other stuff, but I don't want to get into the
he-said-she-said stuff, I'm describing a bug here), but your patch breaks
something that worked previously.

Ronald
Michael Niedermayer Nov. 17, 2016, 9:11 p.m.
On Thu, Nov 17, 2016 at 09:13:55PM +0100, Carl Eugen Hoyos wrote:
> 2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov <atomnuker@gmail.com>:
> > On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> >
> >> Hi!
> >>
> >> Attached patch improves output for some ffv1 files imo.
> >> Current slowdown for the existing decode-line timer is
> >> 2%, I wonder if this can be improved through refactoring.
> >>
> >> Please comment, Carl Eugen
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >>
> > So AFAIK the encoder pushes the values to the LSBs but the decoder didn't
> > shift them back up?
> 
> I don't think the encoder does any shifts here but I may misunderstand.
> 
> > I think you should add a comment explaining that happens.
> 
> Many (older) decoders have to do this and there is nowhere a
> comment, I really believe that this is not particularly convoluted
> code.
> 
> > Also 2% on a decoder doesn't sound that great,
> 
> It's 2% in a function of a decoder.
> 
> > did you try using an if case for the entire loop for when the
> > values need to be shifted?
> 
> That is what I tried to suggest with "refactoring", I suspect
> Michael wasn't too happy about the idea.

can the whole "what to put in the lsb" question be avoided by adding
gray10 support to ffv1dec ?
if so this might be the best solution

[...]
Ronald S. Bultje Nov. 17, 2016, 9:15 p.m.
Hi,

On Thu, Nov 17, 2016 at 4:11 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Thu, Nov 17, 2016 at 09:13:55PM +0100, Carl Eugen Hoyos wrote:
> > 2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov <atomnuker@gmail.com>:
> > > On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at>
> wrote:
> > >
> > >> Hi!
> > >>
> > >> Attached patch improves output for some ffv1 files imo.
> > >> Current slowdown for the existing decode-line timer is
> > >> 2%, I wonder if this can be improved through refactoring.
> > >>
> > >> Please comment, Carl Eugen
> > >>
> > >> _______________________________________________
> > >> ffmpeg-devel mailing list
> > >> ffmpeg-devel@ffmpeg.org
> > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>
> > >>
> > > So AFAIK the encoder pushes the values to the LSBs but the decoder
> didn't
> > > shift them back up?
> >
> > I don't think the encoder does any shifts here but I may misunderstand.
> >
> > > I think you should add a comment explaining that happens.
> >
> > Many (older) decoders have to do this and there is nowhere a
> > comment, I really believe that this is not particularly convoluted
> > code.
> >
> > > Also 2% on a decoder doesn't sound that great,
> >
> > It's 2% in a function of a decoder.
> >
> > > did you try using an if case for the entire loop for when the
> > > values need to be shifted?
> >
> > That is what I tried to suggest with "refactoring", I suspect
> > Michael wasn't too happy about the idea.
>
> can the whole "what to put in the lsb" question be avoided by adding
> gray10 support to ffv1dec ?
> if so this might be the best solution


I believe that would also resolve the issue, yes...

Ronald
Paul B Mahol Nov. 17, 2016, 9:16 p.m.
On 11/17/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Thu, Nov 17, 2016 at 09:13:55PM +0100, Carl Eugen Hoyos wrote:
>> 2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov <atomnuker@gmail.com>:
>> > On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at>
>> > wrote:
>> >
>> >> Hi!
>> >>
>> >> Attached patch improves output for some ffv1 files imo.
>> >> Current slowdown for the existing decode-line timer is
>> >> 2%, I wonder if this can be improved through refactoring.
>> >>
>> >> Please comment, Carl Eugen
>> >>
>> >> _______________________________________________
>> >> ffmpeg-devel mailing list
>> >> ffmpeg-devel@ffmpeg.org
>> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >>
>> >>
>> > So AFAIK the encoder pushes the values to the LSBs but the decoder
>> > didn't
>> > shift them back up?
>>
>> I don't think the encoder does any shifts here but I may misunderstand.
>>
>> > I think you should add a comment explaining that happens.
>>
>> Many (older) decoders have to do this and there is nowhere a
>> comment, I really believe that this is not particularly convoluted
>> code.
>>
>> > Also 2% on a decoder doesn't sound that great,
>>
>> It's 2% in a function of a decoder.
>>
>> > did you try using an if case for the entire loop for when the
>> > values need to be shifted?
>>
>> That is what I tried to suggest with "refactoring", I suspect
>> Michael wasn't too happy about the idea.
>
> can the whole "what to put in the lsb" question be avoided by adding
> gray10 support to ffv1dec ?
> if so this might be the best solution

It is best solution and most logical one imho to support pixel format
on both input and output, expecially for lossless codec, which ffv1 should be.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Old school: Use the lowest level language in which you can solve the
> problem
>             conveniently.
> New school: Use the highest level language in which the latest
> supercomputer
>             can solve the problem without the user falling asleep waiting.
>
Hendrik Leppkes Nov. 17, 2016, 10:05 p.m.
Am 17.11.2016 22:16 schrieb "Paul B Mahol" <onemda@gmail.com>:
>
> On 11/17/16, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Thu, Nov 17, 2016 at 09:13:55PM +0100, Carl Eugen Hoyos wrote:
> >> 2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov <atomnuker@gmail.com>:
> >> > On 16 November 2016 at 11:15, Carl Eugen Hoyos <cehoyos@ag.or.at>
> >> > wrote:
> >> >
> >> >> Hi!
> >> >>
> >> >> Attached patch improves output for some ffv1 files imo.
> >> >> Current slowdown for the existing decode-line timer is
> >> >> 2%, I wonder if this can be improved through refactoring.
> >> >>
> >> >> Please comment, Carl Eugen
> >> >>
> >> >> _______________________________________________
> >> >> ffmpeg-devel mailing list
> >> >> ffmpeg-devel@ffmpeg.org
> >> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >>
> >> >>
> >> > So AFAIK the encoder pushes the values to the LSBs but the decoder
> >> > didn't
> >> > shift them back up?
> >>
> >> I don't think the encoder does any shifts here but I may misunderstand.
> >>
> >> > I think you should add a comment explaining that happens.
> >>
> >> Many (older) decoders have to do this and there is nowhere a
> >> comment, I really believe that this is not particularly convoluted
> >> code.
> >>
> >> > Also 2% on a decoder doesn't sound that great,
> >>
> >> It's 2% in a function of a decoder.
> >>
> >> > did you try using an if case for the entire loop for when the
> >> > values need to be shifted?
> >>
> >> That is what I tried to suggest with "refactoring", I suspect
> >> Michael wasn't too happy about the idea.
> >
> > can the whole "what to put in the lsb" question be avoided by adding
> > gray10 support to ffv1dec ?
> > if so this might be the best solution
>
> It is best solution and most logical one imho to support pixel format
> on both input and output, expecially for lossless codec, which ffv1
should be.
>

Indeed. It makes no sense to encode 10bit and then decode to 16 bit with
random bits added in the LSBs. That is by definition not lossless, and
Ronald has shown above why.

- Hendrik
Tobias Rapp Nov. 18, 2016, 10:09 a.m.
On 17.11.2016 21:34, James Almer wrote:
> On 11/17/2016 5:11 PM, Carl Eugen Hoyos wrote:
>> 2016-11-16 15:02 GMT+01:00 Michael Niedermayer <michael@niedermayer.cc>:
>>> On Wed, Nov 16, 2016 at 12:15:28PM +0100, Carl Eugen Hoyos wrote:
>>>> Hi!
>>>>
>>>> Attached patch improves output for some ffv1 files imo.
>>>> Current slowdown for the existing decode-line timer is
>>>> 2%, I wonder if this can be improved through refactoring.
>>>>
>>>> Please comment, Carl Eugen
>>>
>>>>  ffv1dec.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>> ab0f3ff58a18145efd66d627ecbf54c35b5c23e6  0001-lavc-ffv1dec-Scale-output-for-lsb-packed-compression.patch
>>>> From e4e8277d4f0daeb9f61c2da9ebb167a084b1b93c Mon Sep 17 00:00:00 2001
>>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>>> Date: Wed, 16 Nov 2016 12:03:17 +0100
>>>> Subject: [PATCH] lavc/ffv1dec: Scale output for msb-packed compression to
>>>>  full 16bit.
>>>>
>>>> 2% slowdown for existing decode-line timer.
>>>> ---
>>>>  libavcodec/ffv1dec.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> probably ok
>>>
>>> also try if using a temporary variable  is faster
>>
>> Unfortunately not.
>>
>> Patch applied, Carl Eugen
>
> You pushed it /before/ addressing people's objections and concerns.
>
> A patch that changes the output of a decoder for a codec in the middle
> of being standardized shouldn't be applied just like that. Especially
> when it's a controversial change objected by several developers.

+1

I was really surprised to find this patch committed, especially as the 
mail was tagged "[RFC]" so I thought this is some work-in-progress with 
the real "[PATCH]" to show up later.

Regards,
Tobias

Patch hide | download patch | download mbox

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index d8f35c3..0719e28 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -142,7 +142,7 @@  static void decode_plane(FFV1Context *s, uint8_t *src,
                 }
             } else {
                 for (x = 0; x < w; x++) {
-                    ((uint16_t*)(src + stride*y))[x*pixel_stride] = sample[1][x] << (16 - s->avctx->bits_per_raw_sample);
+                    ((uint16_t*)(src + stride*y))[x*pixel_stride] = sample[1][x] << (16 - s->avctx->bits_per_raw_sample) | ((uint16_t **)sample)[1][x] >> (2 * s->avctx->bits_per_raw_sample - 16);
                 }
             }
         }