[FFmpeg-devel] [PATCH] FFV1 specification: Reduce redundancy in the description of xxPlane() and xxLine()

[Michael Niedermayer]

On Sat, May 02, 2015 at 01:57:17AM +0200, Jerome Martinez wrote:
> New patch because I misunderstood the definition of plane_count.
> Now is 1 + ( ( chroma_planes || version < 4 ) ? 1 : 0 ) + (
> alpha_plane ? 1 : 0 )
> 
> Le 02/05/2015 01:33, Jerome Martinez a écrit :
> >Some notes:
> >- I discarded the "if version >= 4" stuff for grayscale because I
> >don't see such limitation in the bitstream and in the source code.
> >I am thinking to add a specific section about decoder limitations
> >(e.g. bits_per_raw_sample accepted range, gray/alpha support...)
> >- I hesitated to define
> >  * quant_table_index_count = 1 + ( chroma_planes ? 1 : 0 ) + (
> >alpha_plane ? 1 : 0 )
> >  * plane_count = 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane ? 1 : 0 )
> >  but they are nowhere else in the specification, so I direclty
> >put these formulas in the "if".
> >  I think it is a bit verbose in the "if" but has the advantage
> >not to have to define extra parameters and we focus on the
> >bitstream instead.
> >- xxPlanes and xxLines were never defined, so I replace undefined
> >functions by other undefined functions. Planes and Lines functions
> >will be defined in future patches.
> >
> >
> >_______________________________________________
> >ffmpeg-devel mailing list
> >ffmpeg-devel at ffmpeg.org
> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

>  ffv1.lyx |  794 +++------------------------------------------------------------
>  1 file changed, 46 insertions(+), 748 deletions(-)
> 5f46fea9b250e90d66cf86d4fc50daada26238af  0001-Reduce-redundancy-in-the-description-of-xxPlane-and-.patch
> From a70a7132fe56a144a668736cc7864d9dd232d818 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome at mediaarea.net>
> Date: Sat, 2 May 2015 01:53:47 +0200
> Subject: [PATCH] Reduce redundancy in the description of xxPlane() and
>  xxLine().
> 
> LumaPlane(), CbPlane(), CrPlane() and AlphaPlane() are actually same; the order of planes is already defined in the General section and has no impact on the bitstream. Reduced to one Plane( p ) call.
> LumaLine(), CbLine(), CrLine() and AlphaLine() are actually same; the order of lines is already defined in the General section and has no impact on the bitstream. Reduced to one Line( p, y ) call.
> plane_count name may be misleading (it is the count of quant_table_index, which is not always the count of planes) and does not exist in the bistream, replaced by the sum of existing bitstream elements.
> colorspace_type related "if" sorted in ascending order.

i dont think its a good idea to replace a named variable in a
for () statement by a construct so long that it needs a linebreak
in the text output

-+----------------------------------------+------+
-|     for( j = 0; j < plane_count; j++)  |      |
-+----------------------------------------+------+
++--------------------------------------------------------------------------------------------------------+------+
+|     for( j = 0; j < 1 + ( ( chroma_planes || version < 4 ) ? 1 :
+0 ) + ( alpha_plane ? 1 : 0 ); j++)  |      |
++--------------------------------------------------------------------------------------------------------+------+

also a more critical problem is that this patch makes the spec
less well defined.
Theres no ambiguity in what a Luma, Cb, Cr, Alpha something is
but a plane 0 plane 1 plane 2, line 0 line 1 line 2 line 3
which is which ?

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150503/5b92c373/attachment.asc>