[FFmpeg-devel] avcodec/ffv1: Support for RGBA64 and GBRAP16

Submitted by Jerome Martinez on Feb. 1, 2018, 12:43 p.m.

Details

Message ID 63dcf4c8-53bf-ef81-140a-025d6181818c@mediaarea.net
State New
Headers show

Commit Message

Jerome Martinez Feb. 1, 2018, 12:43 p.m.
Add support for 16-bit/component RGB with Alpha encoding and decoding in 
FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding.

Resulting bitstream was tested about lossless encoding/decoding by the 
compression from DPX to FFV1 then decompression from FFV1 to DPX, see 
commands below (resulting framemd5 hashes are all same).
Resulting bitstream is decodable by another decoder (with same resulting 
framemd5 hash).
Resulting bitstream passed through a conformance checker compared to 
current FFV1 specification IETF draft.

About the patch:
- some modified lines are not used (the ones not used when f->use32bit 
is 1), but it makes the code more coherent (especially because 
decode_rgb_frame signature is same for both 16-bit and 32-bit version) 
and prepares the support of RGBA with 10/12/14 bits/component.
- GBRAP16 was chosen for decoding because GBRP16 is already used when no 
alpha, and the code is more prepared for planar pix_fmt when bit depth 
is >8.
- "s->transparency = desc->nb_components == 4 || desc->nb_components == 
2;" is a copy of a line a bit above about the detection of transparency, 
I preferred to reuse it as is even if "YA" 16-bit/component is not (yet) 
supported.

FFmpeg commands used for tests:
./ffmpeg -i in.dpx -c:v ffv1 out.mkv
./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv
./ffmpeg -i out.mkv out.dpx

./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5
./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5
./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5
./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5

Test file used (renamed to in.dpx):
https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx

Jérôme
From 0e149afd7eadb1ec05cc79fff78337b2c543ad8e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net>
Date: Thu, 1 Feb 2018 13:11:53 +0100
Subject: [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16

---
 libavcodec/ffv1dec.c          | 14 ++++++++++----
 libavcodec/ffv1dec_template.c |  6 +++++-
 libavcodec/ffv1enc.c          | 16 ++++++++++++++--
 libavcodec/ffv1enc_template.c | 10 +++++++---
 4 files changed, 36 insertions(+), 10 deletions(-)

Comments

Michael Niedermayer Feb. 2, 2018, 11:10 p.m.
On Thu, Feb 01, 2018 at 01:43:00PM +0100, Jerome Martinez wrote:
> Add support for 16-bit/component RGB with Alpha encoding and decoding in
> FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding.
> 
> Resulting bitstream was tested about lossless encoding/decoding by the
> compression from DPX to FFV1 then decompression from FFV1 to DPX, see
> commands below (resulting framemd5 hashes are all same).
> Resulting bitstream is decodable by another decoder (with same resulting
> framemd5 hash).
> Resulting bitstream passed through a conformance checker compared to current
> FFV1 specification IETF draft.
> 
> About the patch:
> - some modified lines are not used (the ones not used when f->use32bit is
> 1), but it makes the code more coherent (especially because decode_rgb_frame
> signature is same for both 16-bit and 32-bit version) and prepares the
> support of RGBA with 10/12/14 bits/component.
> - GBRAP16 was chosen for decoding because GBRP16 is already used when no
> alpha, and the code is more prepared for planar pix_fmt when bit depth is
> >8.
> - "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;"
> is a copy of a line a bit above about the detection of transparency, I
> preferred to reuse it as is even if "YA" 16-bit/component is not (yet)
> supported.
> 
> FFmpeg commands used for tests:
> ./ffmpeg -i in.dpx -c:v ffv1 out.mkv
> ./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv
> ./ffmpeg -i out.mkv out.dpx
> 
> ./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5
> ./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5
> ./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5
> ./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5
> 
> Test file used (renamed to in.dpx):
> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx

I would prefer if the algorithm would be tuned to 16bit data before
adding more formats to the encoder which require all decoders to support
them.

Dont you agree that this would be the better strategy ?

[...]
Reto Kromer Feb. 3, 2018, 8:10 a.m.
Michael Niedermayer wrote:

>I would prefer if the algorithm would be tuned to 16bit data
>before adding more formats to the encoder which require all
>decoders to support them.
>
>Dont you agree that this would be the better strategy ?

To my understanding FFV1 v3 is actually stable. Or am I missing
something?

Some of us are indeed trying to work on v4, but this is another
topic, I guess.

Best regards, Reto
Jerome Martinez Feb. 3, 2018, 9:57 a.m.
On 03/02/2018 00:10, Michael Niedermayer wrote:
> On Thu, Feb 01, 2018 at 01:43:00PM +0100, Jerome Martinez wrote:
>> Add support for 16-bit/component RGB with Alpha encoding and decoding in
>> FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding.
>>
>> Resulting bitstream was tested about lossless encoding/decoding by the
>> compression from DPX to FFV1 then decompression from FFV1 to DPX, see
>> commands below (resulting framemd5 hashes are all same).
>> Resulting bitstream is decodable by another decoder (with same resulting
>> framemd5 hash).
>> Resulting bitstream passed through a conformance checker compared to current
>> FFV1 specification IETF draft.
>>
>> About the patch:
>> - some modified lines are not used (the ones not used when f->use32bit is
>> 1), but it makes the code more coherent (especially because decode_rgb_frame
>> signature is same for both 16-bit and 32-bit version) and prepares the
>> support of RGBA with 10/12/14 bits/component.
>> - GBRAP16 was chosen for decoding because GBRP16 is already used when no
>> alpha, and the code is more prepared for planar pix_fmt when bit depth is
>>> 8.
>> - "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;"
>> is a copy of a line a bit above about the detection of transparency, I
>> preferred to reuse it as is even if "YA" 16-bit/component is not (yet)
>> supported.
>>
>> FFmpeg commands used for tests:
>> ./ffmpeg -i in.dpx -c:v ffv1 out.mkv
>> ./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv
>> ./ffmpeg -i out.mkv out.dpx
>>
>> ./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5
>> ./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5
>> ./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5
>> ./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5
>>
>> Test file used (renamed to in.dpx):
>> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
> I would prefer if the algorithm would be tuned to 16bit data before
> adding more formats to the encoder which require all decoders to support
> them.
>
> Dont you agree that this would be the better strategy ?

ccing CELLAR.

My remarks are the same as with RGB48 support (including that the 
compression performance of RGB48 so RGBA64 is already very good without 
touching on the algorithm, and IMO tuning should be for v4 for all bit 
depths), with addition that since the last debate on that on 
ffmpeg-devel there was no patch proposal so no consensus on CELLAR for 
limiting the specifications to what exists in FFmpeg implementation (so 
current consensus is that FFV1 specs are for all bit depths for all 
supported color spaces), and since the last debate FFV1 specs draft were 
sent to IETF tracker so it is close to the end.

This patch is just adding the support of RGBA64 conforming to the 
current specs and without big changes (no complex stuff, just mapping to 
the right pix_fmt), and the current specs are the ones with very high 
chances to be the standard (up to now nobody suggested on CELLAR, the 
place for the spec, to limit the support to a set of bit depths / color 
spaces, and nobody suggested a tuning for some bit depths), with the 
main advantage that the specs are clear about all bit depths for all 
color spaces supported (it is good that it is generic). Will this patch 
be accepted after the IETF flags the current specs as stable if there is 
no changes on the wording about the support of all bit depths?

on my side, I can not spend time on FFV1 v4 R&D (tuning and more) when I 
spend time with such blocking (for me) issue about v3 (i.e. agreement in 
specs draft on all bit depths for all supported color spaces but no 
agreement in practice on all bit depths for all supported color spaces).

So for answering directly to the question, no I don't agree that 
changing v3 bitstream with specific tuning of the bitstream depending of 
the bit depth is a better strategy, actually this is the opposite (I 
think that the best strategy is to support as many bit depths as 
possible in implementations with current v3 specs and that all tuning 
should be in the version flagged as experimental, not the one flagged as 
stable even before working on IETF version, if we change a bitstream 
marked as stable we break the trust in the spec being stable, IMO any 
tuning of the bitstream should be done in v4, and any performance 
improvement without breaking the bitstream could be done after this 
patch without problem).

Jérôme
Michael Niedermayer Feb. 3, 2018, 12:07 p.m.
On Sat, Feb 03, 2018 at 10:57:44AM +0100, Jerome Martinez wrote:
> On 03/02/2018 00:10, Michael Niedermayer wrote:
> >On Thu, Feb 01, 2018 at 01:43:00PM +0100, Jerome Martinez wrote:
> >>Add support for 16-bit/component RGB with Alpha encoding and decoding in
> >>FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding.
> >>
> >>Resulting bitstream was tested about lossless encoding/decoding by the
> >>compression from DPX to FFV1 then decompression from FFV1 to DPX, see
> >>commands below (resulting framemd5 hashes are all same).
> >>Resulting bitstream is decodable by another decoder (with same resulting
> >>framemd5 hash).
> >>Resulting bitstream passed through a conformance checker compared to current
> >>FFV1 specification IETF draft.
> >>
> >>About the patch:
> >>- some modified lines are not used (the ones not used when f->use32bit is
> >>1), but it makes the code more coherent (especially because decode_rgb_frame
> >>signature is same for both 16-bit and 32-bit version) and prepares the
> >>support of RGBA with 10/12/14 bits/component.
> >>- GBRAP16 was chosen for decoding because GBRP16 is already used when no
> >>alpha, and the code is more prepared for planar pix_fmt when bit depth is
> >>>8.
> >>- "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;"
> >>is a copy of a line a bit above about the detection of transparency, I
> >>preferred to reuse it as is even if "YA" 16-bit/component is not (yet)
> >>supported.
> >>
> >>FFmpeg commands used for tests:
> >>./ffmpeg -i in.dpx -c:v ffv1 out.mkv
> >>./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv
> >>./ffmpeg -i out.mkv out.dpx
> >>
> >>./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5
> >>./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5
> >>./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5
> >>./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5
> >>
> >>Test file used (renamed to in.dpx):
> >>https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
> >I would prefer if the algorithm would be tuned to 16bit data before
> >adding more formats to the encoder which require all decoders to support
> >them.
> >
> >Dont you agree that this would be the better strategy ?
> 
> ccing CELLAR.
> 
> My remarks are the same as with RGB48 support (including that the
> compression performance of RGB48 so RGBA64 is already very good without
> touching on the algorithm, and IMO tuning should be for v4 for all bit
> depths), with addition that since the last debate on that on ffmpeg-devel
> there was no patch proposal so no consensus on CELLAR for limiting the
> specifications to what exists in FFmpeg implementation (so current consensus
> is that FFV1 specs are for all bit depths for all supported color spaces),
> and since the last debate FFV1 specs draft were sent to IETF tracker so it
> is close to the end.
> 
> This patch is just adding the support of RGBA64 conforming to the current
> specs and without big changes (no complex stuff, just mapping to the right
> pix_fmt), and the current specs are the ones with very high chances to be
> the standard (up to now nobody suggested on CELLAR, the place for the spec,
> to limit the support to a set of bit depths / color spaces, and nobody
> suggested a tuning for some bit depths), with the main advantage that the
> specs are clear about all bit depths for all color spaces supported (it is
> good that it is generic). Will this patch be accepted after the IETF flags
> the current specs as stable if there is no changes on the wording about the
> support of all bit depths?
> 

> on my side, I can not spend time on FFV1 v4 R&D (tuning and more) when I
> spend time with such blocking (for me) issue about v3 (i.e. agreement in
> specs draft on all bit depths for all supported color spaces but no
> agreement in practice on all bit depths for all supported color spaces).
> 

> So for answering directly to the question, no I don't agree that changing v3
> bitstream with specific tuning of the bitstream depending of the bit depth
> is a better strategy,

That was not meant.
To clarify my suggestion,
the algorithm should be tuned for high bit depth before using it for long term
storage. This would be v4 (or later).
Personally i would wait for v4 and not use v3 for high bit depth. Which is
why i think its not smart to extend the v3 implementation with more high depth
support.


> actually this is the opposite (I think that the best
> strategy is to support as many bit depths as possible in implementations
> with current v3 specs and that all tuning should be in the version flagged
> as experimental, not the one flagged as stable even before working on IETF
> version, if we change a bitstream marked as stable we break the trust in the
> spec being stable, IMO any tuning of the bitstream should be done in v4, and
> any performance improvement without breaking the bitstream could be done
> after this patch without problem).

IIRC teh v3 draft was intended to describe the format generated by the existing
implementation. The existing implementation allowed 16bit RGB only when the user
specified flags indicating that they are creating non standard files.
"16bit RGB is experimental and under development, only use it for experiments"
There was no standard 16bit RGB or higher depth supported, these where in 
development on the specification and implementation side.

IIUC people created such files somehow beliving that the IMO clear warning
somehow suggested that this was stable. And with that we are a bit stuck
with this for v3

you are technically correct that the current draft allows it and the additions
but iam not sure its really a good idea to add more cases under this.

Thanks

[...]
Reto Kromer Feb. 3, 2018, 12:58 p.m.
Michael Niedermayer wrote:

>To clarify my suggestion,
>the algorithm should be tuned for high bit depth before using
>it for long term storage. This would be v4 (or later).
>Personally i would wait for v4 and not use v3 for high bit
>depth. Which is why i think its not smart to extend the v3
>implementation with more high depth support.

The issue is that in the real world we need to use the format
now. Otherwise the film archives must use MXF/DPX instead of
Матрёшка/FFV1. That's the point!

I presented to the industry this solution in August 2016 at "The
Reel Thing" technical symposium in Hollywood, and an article on
it was published in April 2017 in FIAF's "Journal of Film
Preservation". The archives cannot wait forever! And they are
indeed important users of FFmpeg, in my opinion.

I already paid EUR 7000 for the FFV1 use in the archival world,
and will pay EUR 5000 additionally in the next months.

>IIUC people created such files somehow beliving that the IMO
>clear warning somehow suggested that this was stable. And with
>that we are a bit stuck with this for v3

I presented last November at the "No Time to Wait" symposium
(nomen est omen) in Vienna - i.e. in your city - how we have to
work today and how we hope to modify container and codec during
the next data migration.

Last but not least, since almost two years now it's impossible
to work on the development of FFV1 v4. It's always the wrong
time and/or the wrong place every time I am doing something for
this cause. Why? This is extremely frustrating.

Best regards, Reto
Michael Niedermayer Feb. 3, 2018, 1:48 p.m.
On Sat, Feb 03, 2018 at 01:58:10PM +0100, Reto Kromer wrote:
> Michael Niedermayer wrote:
> 
> >To clarify my suggestion,
> >the algorithm should be tuned for high bit depth before using
> >it for long term storage. This would be v4 (or later).
> >Personally i would wait for v4 and not use v3 for high bit
> >depth. Which is why i think its not smart to extend the v3
> >implementation with more high depth support.
> 
> The issue is that in the real world we need to use the format
> now. Otherwise the film archives must use MXF/DPX instead of
> Матрёшка/FFV1. That's the point!

Then using the v3 16bit is probably the most realistic option.
And jeromes patch should probably be applied

I hope this will not reduce interrest in working on a improved 
9-16bit mode in v4.


[...]

> Last but not least, since almost two years now it's impossible
> to work on the development of FFV1 v4. It's always the wrong
> time and/or the wrong place every time I am doing something for
> this cause. Why? This is extremely frustrating.

I want to understand this too. IMO v4 development should be more
important than or at least equally to the v3 draft polishing and neither 
should block the other.
Jean-Christophe Kummer Feb. 3, 2018, 2:49 p.m.
Loooking to it from a pragmatic management point of view, there is a lot of open source unfriendly community from the EBU/SMPTE world which would have their opinion about a failed or even more delayed IETF spec finalisation.

I think - given the efforts which have been put into it, the beauty with what  is ALREADY existing, 
coexistency of v4 work and finalisation of v3 work towards IETF standardization is the strategy which 
we should concentrate on.  

16bit in v3: Please go for it. And allow for v4 for other discussions. 

Not to forget about all video projects which would be happy to see that IETF and the community finalises v3 also for it´s  8/10bit formats which have an even higher representation in hundred thousands of archive hours already now. 


If there is something I can do please contact me.


Christophe
NOA




-----Ursprüngliche Nachricht-----
Von: Cellar [mailto:cellar-bounces@ietf.org] Im Auftrag von Michael Niedermayer
Gesendet: Samstag, 03. Februar 2018 14:48
An: Codec Encoding for LossLess Archiving and Realtime transmission <cellar@ietf.org>
Cc: ffmpeg-devel@ffmpeg.org
Betreff: Re: [Cellar] [FFmpeg-devel] [PATCH] avcodec/ffv1: Support for RGBA64 and GBRAP16

On Sat, Feb 03, 2018 at 01:58:10PM +0100, Reto Kromer wrote:
> Michael Niedermayer wrote:

> 

> >To clarify my suggestion,

> >the algorithm should be tuned for high bit depth before using it for 

> >long term storage. This would be v4 (or later).

> >Personally i would wait for v4 and not use v3 for high bit depth. 

> >Which is why i think its not smart to extend the v3 implementation 

> >with more high depth support.

> 

> The issue is that in the real world we need to use the format now. 

> Otherwise the film archives must use MXF/DPX instead of Матрёшка/FFV1. 

> That's the point!


Then using the v3 16bit is probably the most realistic option.
And jeromes patch should probably be applied

I hope this will not reduce interrest in working on a improved 9-16bit mode in v4.


[...]

> Last but not least, since almost two years now it's impossible

> to work on the development of FFV1 v4. It's always the wrong

> time and/or the wrong place every time I am doing something for

> this cause. Why? This is extremely frustrating.


I want to understand this too. IMO v4 development should be more
important than or at least equally to the v3 draft polishing and neither 
should block the other.


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
Michael Niedermayer Feb. 4, 2018, 2 p.m.
On Thu, Feb 01, 2018 at 01:43:00PM +0100, Jerome Martinez wrote:
> Add support for 16-bit/component RGB with Alpha encoding and decoding in
> FFV1, both RGBA64 and GBRAP16 for encoding, GBRAP16 for decoding.
> 
> Resulting bitstream was tested about lossless encoding/decoding by the
> compression from DPX to FFV1 then decompression from FFV1 to DPX, see
> commands below (resulting framemd5 hashes are all same).
> Resulting bitstream is decodable by another decoder (with same resulting
> framemd5 hash).
> Resulting bitstream passed through a conformance checker compared to current
> FFV1 specification IETF draft.
> 
> About the patch:
> - some modified lines are not used (the ones not used when f->use32bit is
> 1), but it makes the code more coherent (especially because decode_rgb_frame
> signature is same for both 16-bit and 32-bit version) and prepares the
> support of RGBA with 10/12/14 bits/component.
> - GBRAP16 was chosen for decoding because GBRP16 is already used when no
> alpha, and the code is more prepared for planar pix_fmt when bit depth is
> >8.
> - "s->transparency = desc->nb_components == 4 || desc->nb_components == 2;"
> is a copy of a line a bit above about the detection of transparency, I
> preferred to reuse it as is even if "YA" 16-bit/component is not (yet)
> supported.
> 
> FFmpeg commands used for tests:
> ./ffmpeg -i in.dpx -c:v ffv1 out.mkv
> ./ffmpeg -i in.dpx -pix_fmt gbrap16 -strict -2 -c:v ffv1 out2.mkv
> ./ffmpeg -i out.mkv out.dpx
> 
> ./ffmpeg -i in.dpx -f framemd5 in.dpx.framemd5
> ./ffmpeg -i out.mkv -pix_fmt rgba64be -f framemd5 out.mkv.framemd5
> ./ffmpeg -i out2.mkv -pix_fmt rgba64be -f framemd5 out2.mkv.framemd5
> ./ffmpeg -i out.dpx -f framemd5 out.dpx.framemd5
> 
> Test file used (renamed to in.dpx):
> https://mediaarea.net/temp/uncropped_DPX_4K_16bit_Overscan15pros.dpx
> 
> Jérôme
> 

[...]
> diff --git a/libavcodec/ffv1enc_template.c b/libavcodec/ffv1enc_template.c
> index b7eea0dd70..2763082540 100644
> --- a/libavcodec/ffv1enc_template.c
> +++ b/libavcodec/ffv1enc_template.c
> @@ -122,8 +122,8 @@ static av_always_inline int RENAME(encode_line)(FFV1Context *s, int w,
>      return 0;
>  }
>  
> -static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3],
> -                                    int w, int h, const int stride[3])
> +static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[4],
> +                                    int w, int h, const int stride[4])
>  {
>      int x, y, p, i;
>      const int ring_size = s->context_model ? 3 : 2;
> @@ -152,14 +152,18 @@ static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3],
>                  r = (v >> 16) & 0xFF;
>                  a =  v >> 24;
>              } else if (packed) {
> -                const uint16_t *p = ((const uint16_t*)(src[0] + x*6 + stride[0]*y));
> +                const uint16_t *p = ((const uint16_t*)(src[0] + x*(3 + s->transparency)*2 + stride[0]*y));
>                  r = p[0];
>                  g = p[1];
>                  b = p[2];

> +                if (s->transparency)

transparency should possibly be moved into a local variable


> +                  a = p[3];
>              } else if (sizeof(TYPE) == 4) {
>                  g = *((const uint16_t *)(src[0] + x*2 + stride[0]*y));
>                  b = *((const uint16_t *)(src[1] + x*2 + stride[1]*y));
>                  r = *((const uint16_t *)(src[2] + x*2 + stride[2]*y));
> +                if (s->transparency)
> +                    a = *((const uint16_t *)(src[3] + x*2 + stride[2]*y));
                                                               ^^^^^^^^^
this looks wrong


also what speed effect do thes changes to the inner loop have ?

[...]
Jerome Martinez Feb. 5, 2018, 9:20 p.m.
On 03/02/2018 14:48, Michael Niedermayer wrote:
>
> I hope this will not reduce interrest in working on a improved
> 9-16bit mode in v4.

I don't like to put politics in technical stuff, but here this is 
politics, and I think that blocking an easy improvement of FFV1 v3 
encoding/decoding in FFmpeg (actually just using the current FFV1 v3, 
and also v1 actually, specification without having artificial 
limitations about bit depths for a specific color space, i.e. 16-bit 
support was already there for YUV before I work on FFV1) just for 
forcing people to do a completely different work (e.g. working on FFV1 
v4) is not fair and IMO is not in the idea behind open source.
IMO if interest for 9-16bit (side note: I don't see why 8-bit should not 
be in the range, some ideas I have for v4 are useful also for 8-bit) 
mode in v4 is reduced, that would only mean that v3 is already so great 
and/or just that people have no better idea right now, it is not bad, 
and both works (using v3 at the maximum of its possibilities and working 
on v4) are separate works. FYI criticism I saw about FFV1 is not 
compression ratio but speed (playing a 4K stream is just impossible on a 
"normal" machine, you need a very expensive CPU for that) so my plan is 
to focus on speed improvement in priority. It could be v4 stuff 
(incompatible changes), or v3 (encoder/decoder optimization), depending 
of what I find.

>
>
> [...]
>
>> Last but not least, since almost two years now it's impossible
>> to work on the development of FFV1 v4. It's always the wrong
>> time and/or the wrong place every time I am doing something for
>> this cause. Why? This is extremely frustrating.
> I want to understand this too. IMO v4 development should be more
> important than or at least equally to the v3 draft polishing and neither
> should block the other.

Also politics, but I don't understand who is blocking v4 from your point 
of view. "impossible to work on v4"? Where? As far as I see 1 person 
(me) said his priority is having FFV1 v3 becoming a standard (so IETF 
related work) and widely used (so any bit depth for any supported color 
space in v3 because it is an easy demonstration that FFV1 is versatile 
without having to wait for v4 R&D, especially because v3 bitstream 
design is already pretty good and versatile) because this is what I 
need, and my personal case is not blocking anyone else for sending 
patches for either FFV1 specifications or FFmpeg code about v4. Eager to 
see patch proposals for v4 (and v3 if it does not break support of files 
in the wild), whoever feeling blocked with his patches should send patch 
proposals to either FFmpeg (code) or CELLAR mailing list (bitstream format).

That said, I plan to add more pix_fmt support for FFV1 v3 (which is 
already a good format!) support in FFmpeg and some optimization ideas 
beside my work for IETF spec, with the hope we could speak about 
technical issues (e.g. more optimization hints or info about wrong code 
or warning about that it breaks the bitstream specs as currently 
written), leaving aside politics.
Dave Rice Feb. 5, 2018, 10:03 p.m.
> On Feb 5, 2018, at 4:20 PM, Jerome Martinez <Jerome@MediaArea.net> wrote:
> 
> On 03/02/2018 14:48, Michael Niedermayer wrote:
>> 
>> I hope this will not reduce interrest in working on a improved
>> 9-16bit mode in v4.
> 
> I don't like to put politics in technical stuff, but here this is politics, and I think that blocking an easy improvement of FFV1 v3 encoding/decoding in FFmpeg (actually just using the current FFV1 v3, and also v1 actually, specification without having artificial limitations about bit depths for a specific color space, i.e. 16-bit support was already there for YUV before I work on FFV1) just for forcing people to do a completely different work (e.g. working on FFV1 v4) is not fair and IMO is not in the idea behind open source.
> IMO if interest for 9-16bit (side note: I don't see why 8-bit should not be in the range, some ideas I have for v4 are useful also for 8-bit) mode in v4 is reduced, that would only mean that v3 is already so great and/or just that people have no better idea right now, it is not bad, and both works (using v3 at the maximum of its possibilities and working on v4) are separate works. FYI criticism I saw about FFV1 is not compression ratio but speed (playing a 4K stream is just impossible on a "normal" machine, you need a very expensive CPU for that) so my plan is to focus on speed improvement in priority. It could be v4 stuff (incompatible changes), or v3 (encoder/decoder optimization), depending of what I find.

IMHO, improved higher bit depths in version 4 is very interesting. We already have a few noted exceptions where version 4 is intended to fix and optimize a few things of version 3 (signed 16 bit handling, RGB plane order for 9-15 bits), so a change in optimization of high bit depth handling seems already appropriate for consideration.

>> [...]
>> 
>>> Last but not least, since almost two years now it's impossible
>>> to work on the development of FFV1 v4. It's always the wrong
>>> time and/or the wrong place every time I am doing something for
>>> this cause. Why? This is extremely frustrating.
>> I want to understand this too. IMO v4 development should be more
>> important than or at least equally to the v3 draft polishing and neither
>> should block the other.
> 
> Also politics, but I don't understand who is blocking v4 from your point of view. "impossible to work on v4"? Where? As far as I see 1 person (me) said his priority is having FFV1 v3 becoming a standard (so IETF related work) and widely used (so any bit depth for any supported color space in v3 because it is an easy demonstration that FFV1 is versatile without having to wait for v4 R&D, especially because v3 bitstream design is already pretty good and versatile) because this is what I need, and my personal case is not blocking anyone else for sending patches for either FFV1 specifications or FFmpeg code about v4. Eager to see patch proposals for v4 (and v3 if it does not break support of files in the wild), whoever feeling blocked with his patches should send patch proposals to either FFmpeg (code) or CELLAR mailing list (bitstream format).
> 
> That said, I plan to add more pix_fmt support for FFV1 v3 (which is already a good format!) support in FFmpeg and some optimization ideas beside my work for IETF spec, with the hope we could speak about technical issues (e.g. more optimization hints or info about wrong code or warning about that it breaks the bitstream specs as currently written), leaving aside politics.

In the cellar charter and timeline, the effort to produce a informational specification for FFV1 video codec versions 0, 1 and 3 precedes the effort to produce a specification for FFV1 video codec version 4. Initially I anticipated that the version 4 specification would be a fork from the completed version 0,1,3 draft; however, I think the current approach (one doc that ‘makes’ both outputs) works well to allow version 4 work to proceed without blocking any remaining version 0,1,3 work. Still I suggest that we resolve https://github.com/FFmpeg/FFV1/pull/87 and https://github.com/FFmpeg/FFV1/pull/71 soon as IMHO the version 0,1,3 informational specification is very close to a last call and submission to the IESG (chairs are welcome to offer other opinions ;) ). So while I encourage resolution to these pull requests, it seems we have a good system to manage concurrent work on both FFV1 goals of the charter.

Kind Regards,
Dave Rice

Patch hide | download patch | download mbox

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 5eadb6b158..923b79f3ab 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -336,14 +336,16 @@  static int decode_slice(AVCodecContext *c, void *arg)
          decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0]    , width, height, p->linesize[0], 0, 2);
          decode_plane(fs, p->data[0] + ps*x + y*p->linesize[0] + 1, width, height, p->linesize[0], 1, 2);
     } else if (f->use32bit) {
-        uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0],
+        uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0],
                                p->data[1] + ps * x + y * p->linesize[1],
-                               p->data[2] + ps * x + y * p->linesize[2] };
+                               p->data[2] + ps * x + y * p->linesize[2],
+                               p->data[3] + ps * x + y * p->linesize[3] };
         decode_rgb_frame32(fs, planes, width, height, p->linesize);
     } else {
-        uint8_t *planes[3] = { p->data[0] + ps * x + y * p->linesize[0],
+        uint8_t *planes[4] = { p->data[0] + ps * x + y * p->linesize[0],
                                p->data[1] + ps * x + y * p->linesize[1],
-                               p->data[2] + ps * x + y * p->linesize[2] };
+                               p->data[2] + ps * x + y * p->linesize[2],
+                               p->data[3] + ps * x + y * p->linesize[3] };
         decode_rgb_frame(fs, planes, width, height, p->linesize);
     }
     if (fs->ac != AC_GOLOMB_RICE && f->version > 2) {
@@ -694,6 +696,10 @@  static int read_header(FFV1Context *f)
             f->avctx->pix_fmt = AV_PIX_FMT_GBRP16;
             f->use32bit = 1;
         }
+        else if (f->avctx->bits_per_raw_sample == 16 && f->transparency) {
+            f->avctx->pix_fmt = AV_PIX_FMT_GBRAP16;
+            f->use32bit = 1;
+        }
     } else {
         av_log(f->avctx, AV_LOG_ERROR, "colorspace not supported\n");
         return AVERROR(ENOSYS);
diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c
index 37df766773..2904a44112 100644
--- a/libavcodec/ffv1dec_template.c
+++ b/libavcodec/ffv1dec_template.c
@@ -107,7 +107,7 @@  static av_always_inline int RENAME(decode_line)(FFV1Context *s, int w,
     return 0;
 }
 
-static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[3], int w, int h, int stride[3])
+static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[4], int w, int h, int stride[4])
 {
     int x, y, p;
     TYPE *sample[4][2];
@@ -158,10 +158,14 @@  static void RENAME(decode_rgb_frame)(FFV1Context *s, uint8_t *src[3], int w, int
                 *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = g;
                 *((uint16_t*)(src[1] + x*2 + stride[1]*y)) = b;
                 *((uint16_t*)(src[2] + x*2 + stride[2]*y)) = r;
+                if (s->transparency)
+                    *((uint16_t*)(src[3] + x*2 + stride[3]*y)) = a;
             } else {
                 *((uint16_t*)(src[0] + x*2 + stride[0]*y)) = b;
                 *((uint16_t*)(src[1] + x*2 + stride[1]*y)) = g;
                 *((uint16_t*)(src[2] + x*2 + stride[2]*y)) = r;
+                if (s->transparency)
+                    *((uint16_t*)(src[3] + x*2 + stride[3]*y)) = a;
             }
         }
     }
diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c
index c0c1558ffe..9ba5c6fcfd 100644
--- a/libavcodec/ffv1enc.c
+++ b/libavcodec/ffv1enc.c
@@ -624,6 +624,14 @@  FF_ENABLE_DEPRECATION_WARNINGS
         s->chroma_planes = 1;
         s->bits_per_raw_sample = 8;
         break;
+    case AV_PIX_FMT_RGBA64:
+        s->colorspace = 1;
+        s->transparency = 1;
+        s->chroma_planes = 1;
+        s->bits_per_raw_sample = 16;
+        s->use32bit = 1;
+        s->version = FFMAX(s->version, 1);
+        break;
     case AV_PIX_FMT_RGB48:
         s->colorspace = 1;
         s->chroma_planes = 1;
@@ -649,10 +657,12 @@  FF_ENABLE_DEPRECATION_WARNINGS
         if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample)
             s->bits_per_raw_sample = 14;
     case AV_PIX_FMT_GBRP16:
+    case AV_PIX_FMT_GBRAP16:
         if (!avctx->bits_per_raw_sample && !s->bits_per_raw_sample)
             s->bits_per_raw_sample = 16;
         else if (!s->bits_per_raw_sample)
             s->bits_per_raw_sample = avctx->bits_per_raw_sample;
+        s->transparency = desc->nb_components == 4 || desc->nb_components == 2;
         s->colorspace = 1;
         s->chroma_planes = 1;
         if (s->bits_per_raw_sample >= 16) {
@@ -1028,9 +1038,10 @@  static int encode_slice(AVCodecContext *c, void *arg)
     const int ps     = av_pix_fmt_desc_get(c->pix_fmt)->comp[0].step;
     int ret;
     RangeCoder c_bak = fs->c;
-    const uint8_t *planes[3] = {p->data[0] + ps*x + y*p->linesize[0],
+    const uint8_t *planes[4] = {p->data[0] + ps*x + y*p->linesize[0],
                                 p->data[1] ? p->data[1] + ps*x + y*p->linesize[1] : NULL,
-                                p->data[2] ? p->data[2] + ps*x + y*p->linesize[2] : NULL};
+                                p->data[2] ? p->data[2] + ps*x + y*p->linesize[2] : NULL,
+                                p->data[3] ? p->data[3] + ps*x + y*p->linesize[3] : NULL};
 
     fs->slice_coding_mode = 0;
     if (f->version > 3) {
@@ -1322,6 +1333,7 @@  AVCodec ff_ffv1_encoder = {
         AV_PIX_FMT_YA8,
         AV_PIX_FMT_GRAY10, AV_PIX_FMT_GRAY12,
         AV_PIX_FMT_GBRP16, AV_PIX_FMT_RGB48,
+        AV_PIX_FMT_GBRAP16, AV_PIX_FMT_RGBA64,
         AV_PIX_FMT_NONE
 
     },
diff --git a/libavcodec/ffv1enc_template.c b/libavcodec/ffv1enc_template.c
index b7eea0dd70..2763082540 100644
--- a/libavcodec/ffv1enc_template.c
+++ b/libavcodec/ffv1enc_template.c
@@ -122,8 +122,8 @@  static av_always_inline int RENAME(encode_line)(FFV1Context *s, int w,
     return 0;
 }
 
-static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3],
-                                    int w, int h, const int stride[3])
+static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[4],
+                                    int w, int h, const int stride[4])
 {
     int x, y, p, i;
     const int ring_size = s->context_model ? 3 : 2;
@@ -152,14 +152,18 @@  static int RENAME(encode_rgb_frame)(FFV1Context *s, const uint8_t *src[3],
                 r = (v >> 16) & 0xFF;
                 a =  v >> 24;
             } else if (packed) {
-                const uint16_t *p = ((const uint16_t*)(src[0] + x*6 + stride[0]*y));
+                const uint16_t *p = ((const uint16_t*)(src[0] + x*(3 + s->transparency)*2 + stride[0]*y));
                 r = p[0];
                 g = p[1];
                 b = p[2];
+                if (s->transparency)
+                  a = p[3];
             } else if (sizeof(TYPE) == 4) {
                 g = *((const uint16_t *)(src[0] + x*2 + stride[0]*y));
                 b = *((const uint16_t *)(src[1] + x*2 + stride[1]*y));
                 r = *((const uint16_t *)(src[2] + x*2 + stride[2]*y));
+                if (s->transparency)
+                    a = *((const uint16_t *)(src[3] + x*2 + stride[2]*y));
             } else {
                 b = *((const uint16_t *)(src[0] + x*2 + stride[0]*y));
                 g = *((const uint16_t *)(src[1] + x*2 + stride[1]*y));