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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180203/c3fa0358/attachment.sig>