| Message ID | 201611161215.28889.cehoyos@ag.or.at |
|---|---|
| State | New |
| Headers | show |
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
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
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 [...]
> > 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
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?
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
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
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
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
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
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
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
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
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".
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.
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.
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.
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
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 [...]
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
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. >
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
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
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); } } }
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(-)