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

Jerome Martinez jerome at mediaarea.net
Sun May 3 12:31:05 CEST 2015


Le 03/05/2015 04:57, Michael Niedermayer a écrit :
> 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 [...]

I hesitated and this is a very good point, so I replaced by 2 named 
variables:
- plane_count which is the count of planes
- quant_table_index_count which is the count of quant_table indexes

Note that plane_count in the ffv1 source code is actually the count of 
quant_table indexes, and is _not_ the actual count of planes, I think it 
is dangerous to have this name for the count of quant_table indexes and 
I'll propose a ffv1 source code patch for renaming it.

>
> also a more critical problem is that this patch makes the spec
> less well defined.

It does not.
As written in the patch description:
"the order of planes is already defined in the General section and has 
no impact on the bitstream."

It is referencing the following sentences in the General section:
"In the case of the normal YCbCr colorspace the Y plane is coded first 
followed by the Cb and Cr planes, if an Alpha/transparency plane exists, 
it is coded last. In the case of the JPEG2000-RCT colorspace the lines 
are interleaved to improve caching efficiency since it is most likely 
that the RCT will immediately be converted to RGB during decoding; the 
interleaved coding order is also Y,Cb,Cr,Alpha."

It is not good to have redundancy in a specification.

Additionally, this is a limitation in the bitstream syntax which should 
not be present from my point of view, because the bitstream does not 
care about the type of the plane: Y, color or alpha, the bitstream 
syntax is same. Plane() and Line() are generic. so the description of 
planes should not be in the bitstream syntax. I don't see how I would 
define LumaPlane, CbPlane, CrPlane, AlphaPlane without repeating 99% of 
the description in each function. And it would limit future extension: 
it would be so easy to add any other color space e.g. XYZ (used by 
Cinema, see DCP packages), RGB, sRGB, CMYK, HSV or HSL, why not creating 
a generic bitstream syntax instead of forcing YUV? Actually, it is not 
the future (see below).

> 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 ?

Actually, if I well understood, there is already an issue with the way 
you describe the bitstream.
In the case of version = 4, colorspace = 1 and slice_coding_mode = 1, 
you describe lines this way:
LumaLine[y]
CbLine[y]
CrLine[y]

but from the source code, I understand that:
BLine[y]
GLine[y]
RLine[y]

if we keep xxLine, code should be transformed from:

LumaLine[y]
CbLine[y]
CrLine[y]

to

if (slice_coding_mode == 0)
{
LumaLine[y]
CbLine[y]
CrLine[y]
}
if (slice_coding_mode == 1)
{
BLine[y]
GLine[y]
RLine[y]
}

so from my point of view your description (in the General section and in 
the bitstream syntax) is already wrong (as well is the definition of 
chroma_planes which should be transformed to "must be 1" in the case of 
colorspace = 1). So this patch fixes one issue (no LumaLine / CbLine / 
CrLine in the case of version = 4, colorspace = 1 and slice_coding_mode 
= 1), and I plan to fix the other issues (wrong description of planes 
and wrong description of chroma_planes in the General section and in in 
the case of version = 4, colorspace = 1 and slice_coding_mode = 1) in a 
future patch.

This is leading to a proposal of bitstream change I would like to 
propose for version 4, but it is maybe better to discuss of this 
proposal in its own thread (and it is not the purpose of this patch).
-------------- next part --------------
>From ba2af678d50c2c3fa089eb9cee5b73da92bc2a3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome at mediaarea.net>
Date: Sun, 3 May 2015 11:16:30 +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.
---
 ffv1.lyx | 801 +++++----------------------------------------------------------
 1 file changed, 53 insertions(+), 748 deletions(-)

diff --git a/ffv1.lyx b/ffv1.lyx
index dd5eb50..07e9348 100644
--- a/ffv1.lyx
+++ b/ffv1.lyx
@@ -2897,7 +2897,7 @@ Slice
 
 \begin_layout Standard
 \begin_inset Tabular
-<lyxtabular version="3" rows="27" columns="2">
+<lyxtabular version="3" rows="18" columns="2">
 <features rotate="0" tabularvalignment="middle">
 <column alignment="left" valignment="top">
 <column alignment="center" valignment="top">
@@ -3008,537 +3008,6 @@ SliceHeader( i )
 </cell>
 </row>
 <row>
-<cell alignment="left" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-if( colorspace_type == 1) {
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="left" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-for( y = 0;
-\begin_inset space ~
-\end_inset
-
-y < height;
-\begin_inset space ~
-\end_inset
-
-y++ ) {
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-LumaLine( y )
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-CbLine( y )
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-CrLine( y )
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-if( alpha_plane )
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-AlphaLine( y )
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-}
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-} else {
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
 <cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
 \begin_inset Text
 
@@ -3558,23 +3027,7 @@ AlphaLine( y )
 \begin_inset space ~
 \end_inset
 
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-LumaPlane( )
+if( colorspace_type == 0) {
 \end_layout
 
 \end_inset
@@ -3625,7 +3078,7 @@ LumaPlane( )
 \begin_inset space ~
 \end_inset
 
-if( chroma_planes ) {
+for( p = 0; p < plane_count; p++ ) {
 \end_layout
 
 \end_inset
@@ -3692,7 +3145,7 @@ if( chroma_planes ) {
 \begin_inset space ~
 \end_inset
 
-CbPlane( )
+Plane( p )
 \end_layout
 
 \end_inset
@@ -3708,7 +3161,7 @@ CbPlane( )
 </cell>
 </row>
 <row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+<cell alignment="left" valignment="top" topline="true" leftline="true" usebox="none">
 \begin_inset Text
 
 \begin_layout Plain Layout
@@ -3727,39 +3180,7 @@ CbPlane( )
 \begin_inset space ~
 \end_inset
 
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-
-\begin_inset space ~
-\end_inset
-
-CrPlane( )
+} else if( colorspace_type == 1) {
 \end_layout
 
 \end_inset
@@ -3775,7 +3196,7 @@ CrPlane( )
 </cell>
 </row>
 <row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
+<cell alignment="left" valignment="top" topline="true" leftline="true" usebox="none">
 \begin_inset Text
 
 \begin_layout Plain Layout
@@ -3810,7 +3231,7 @@ CrPlane( )
 \begin_inset space ~
 \end_inset
 
-}
+for( y = 0; y < height; y++ )
 \end_layout
 
 \end_inset
@@ -3861,7 +3282,23 @@ CrPlane( )
 \begin_inset space ~
 \end_inset
 
-if( alpha_plane )
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+for( p = 0; p < plane_count; p++ ) {
 \end_layout
 
 \end_inset
@@ -3928,7 +3365,23 @@ if( alpha_plane )
 \begin_inset space ~
 \end_inset
 
-AlphaPlane( )
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+
+\begin_inset space ~
+\end_inset
+
+Line( p, y )
 \end_layout
 
 \end_inset
@@ -4264,6 +3717,11 @@ u(32)
 \end_layout
 
 \begin_layout Description
+plane_count is defined as 1 + ( chroma_planes ? 2 : 0 ) + ( alpha_plane
+ ? 1 : 0 ).
+\end_layout
+
+\begin_layout Description
 slice_size indicates the size of the slice in bytes.
 \begin_inset Newline newline
 \end_inset
@@ -4391,164 +3849,6 @@ reserved for future use
 
 \end_layout
 
-\begin_layout Description
-plane_count indicates the count of planes and the associated plane types.
-\begin_inset Newline newline
-\end_inset
-
-
-\begin_inset Tabular
-<lyxtabular version="3" rows="7" columns="2">
-<features rotate="0" tabularvalignment="middle">
-<column alignment="center" valignment="top">
-<column alignment="center" valignment="top">
-<row>
-<cell alignment="center" valignment="top" topline="true" bottomline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-value
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" bottomline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-plane types
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-0
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-forbidden
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-1
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-if version <4: forbidden; else gray
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-2
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-if version <4: forbidden; else gray+alpha
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-3
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-luma+chroma
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-4
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-luma+chroma+alpha
-\end_layout
-
-\end_inset
-</cell>
-</row>
-<row>
-<cell alignment="center" valignment="top" topline="true" bottomline="true" leftline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-Other
-\end_layout
-
-\end_inset
-</cell>
-<cell alignment="center" valignment="top" topline="true" bottomline="true" leftline="true" rightline="true" usebox="none">
-\begin_inset Text
-
-\begin_layout Plain Layout
-reserved for future use
-\end_layout
-
-\end_inset
-</cell>
-</row>
-</lyxtabular>
-
-\end_inset
-
-
-\end_layout
-
 \begin_layout Subsection
 Slice Header
 \end_layout
@@ -4739,7 +4039,7 @@ ur
 \begin_inset space ~
 \end_inset
 
-for( j = 0; j < plane_count; j++)
+for( j = 0; j < quant_table_index_count; j++)
 \end_layout
 
 \end_inset
@@ -5107,6 +4407,11 @@ Inferred to be 1 if not present.
 \end_layout
 
 \begin_layout Description
+quant_table_index_count is defined as 1 + ( ( chroma_planes || version <
+ 4 ) ? 1 : 0 ) + ( alpha_plane ? 1 : 0 ).
+\end_layout
+
+\begin_layout Description
 quant_table_index indicates the index to select the quantization table set
  and the initial states for the slice.
 \begin_inset Newline newline
-- 
1.9.5.msysgit.1



More information about the ffmpeg-devel mailing list