[FFmpeg-devel] [Cellar] [PATCH] FFV1: make RGB48 support as non-experimental

[Michael Niedermayer]

Hi

On Sun, Jan 07, 2018 at 06:39:34PM +0100, Jerome Martinez wrote:
> On 06/01/2018 23:21, Michael Niedermayer wrote:
> >On Sat, Jan 06, 2018 at 04:54:18PM +0100, Jerome Martinez wrote:
> >>On 06/01/2018 02:05, Michael Niedermayer wrote:
> >>>>  ffv1enc.c |    4 ----
> >>>>  1 file changed, 4 deletions(-)
> >>>>acfc60c913b311b148f2eeef2d2d6ea9e37afcf7  0001-avcodec-ffv1enc-mark-RGB48-support-as-non-experiment.patch
> >>>> From 303f63fb7e6172fdb7de66da1f8a4006b79a535f Mon Sep 17 00:00:00 2001
> >>>>From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome at mediaarea.net>
> >>>>Date: Fri, 5 Jan 2018 11:09:01 +0100
> >>>>Subject: [PATCH] avcodec/ffv1enc: mark RGB48 support as non-experimental
> >>>>
> >>>>Resulting bitstream was tested with a conformance checker
> >>>>using the last draft of FFV1 specifications.
> >>>But has the way this is stored been optimized ?
> >>>
> >>>Once its marked as non exerimental all future decoders must support the exact
> >>>way.
> >>Although this is considered experimental in the encoder, the implementation
> >>adheres to the description in the specification. The bitstream specification
> >>does not provide a bitdepth limit so with the current draft of the
> >>specification, another FFV1 encoder could already encode 16-bit (and more)
> >>content. The work on the specification has been careful to not break
> >>compatibility with former drafts and at this point the FFV1 bitstream
> >>specification for versions 0, 1, and 3 should be considered already
> >>non-experimental for all bit depths. All current decoders should already
> >>support the exact way it is currently specified.
> >>
> >>To make a change in the specification at this point would have cascading
> >>consequences as we’d have to retract the part of the specification which
> >>states that micro_version 4 of version 3 is the first stable variant. Worse,
> >>it is impossible to indicate a bitstream change in FFV1 version 1, which
> >>permits RGB 16-bit content too, so it would be difficult for a decoder to
> >>detect a bitstream not conforming to the bitstream created by the current
> >>version of FFmpeg encoder.
> >Are you not making this look alot more complex than it is ?
> >Or maybe we talk about slightly different things here
> >
> >with the next version we can introduce any method of storing 16bit or 9-15 bit
> >and we certainly do not support in the implementation all possible bit depths
> >
> > From what i remember I had always wanted to improve the way that
> >more than 8bit is handled, not just 16. Although 16 is a bit special
> >
> >Consider this:
> >If we improve get_context() in the next version for >8bit
> >we still have to support 9-15 bit with the old definition
> >if we now declare 16bit non experimental then we also must support that with
> >an old get_context() in the decoder.
> >the 16bit path is seperate from the lower bit depth. So this is an Additional
> >codepath that we have to carry in the future
> >
> >isnt it smarter now that if we want to improve get_context() that we
> >dont now extend what can be generated with the current get_context ?
> >
> >or are such current get_context() style files already widespread ?
> >if so then its probably best to accept it and keep supporting it
> 
> I am lost.
> Looks like we talk about 2 different subjects: FFV1 bitstream specifications
> and FFmpeg implementation.
> Let separate subjects:
> 

[...]

> FFmpeg implementation:
> FFV1 YCbCr 16-bit is already flagged as non experimental, so there is
> already some non experimental 16-bit support in FFmpeg FFV1 encoder. 16-bit
> is not new and for RGB48 the actual encoding after RGB to YCbCr
> transformation is just 1 bit more (17-bit).

This is correct but i think misleading a bit
the 17bit case this is about uses a seperate codepath. So if its enabled
we generate new files that have never been generated before in some sense
AFAIK

If we change the bitstream in a future version, which i belive we 
should consider. Then we have an additional "old" version of the 17bit path
that needs to be supported in implementations.


> FFmpeg experimental flag is for the status of the encoder, not for the
> status of the bitstream (the bitstream for versions 0, 1, 3 is already
> considered stable for RGB48 and others)

I think i added the flag check. The reason behind the check was so that the
bitstream syntax can be changed without the need to support every iteration
of the bitstream.
I had hoped someone would fund research and testing of further improvments 
to the various fine details of higher bit depth encoding beyond 8bit.
IIRC No further funding was provided, Noone worked on it as a volunteer,
No changes where proposed  ...

But it wasnt intended as a "bitstream is in stone, encoder might mismatch
the spec" at the time.

We have a draft spec now that does not limit bitdepth in any way, this may
have been a mistakke but i dont see this as a big problem honestly. I do not
propose to change this. But i would not oppose it if people want to change it

If someone creates a 19bit per sample file based on the draft spec it also
will not decode with most real world implementations.
There is no question that with a unlimited bitdepth, real implementations
will never support some depths

I just dont want to create a new variant of files _IF_ we plan to work on
improving the "bitstream syntax" in the next version.
Of course if these files are already out in the wild then we must support
this in the implementation. And then most of this discussion is meaningless

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- 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/20180107/724990d6/attachment.sig>