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

[Jerome Martinez]

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