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

Jerome Martinez jerome at mediaarea.net
Sun May 3 14:59:59 CEST 2015


Le 03/05/2015 13:55, Michael Niedermayer a écrit :
> On Sun, May 03, 2015 at 12:31:05PM +0200, Jerome Martinez wrote:
>> - plane_count which is the count of planes
> thats a bad name for packed formats

Good point, but this is not worse than the previous name used ;-).
and actually you use "alpha_plane" even for color_space = 1 (so no planes)

Attached is a new patch with channel_count name.
"channel" wording taken from 
https://www.ffmpeg.org/ffmpeg-all.html#extractplanes .
should I also change, in another patch, chroma_planes and alpha_plane to 
chroma_channels and alpha_channel for more coherency?

>
>
>> - quant_table_index_count which is the count of quant_table indexes
> any reason not to call it quant_table_count ? (its shorter and seems
> not to contain less information)

quant_table_count is already defined as the count of quantization tables 
(in the header), we can not reuse that name for something different.
here, it is the count of quantization table indexes, which is different 
(it is possible to have 1000 quantization tables, but we can have only 
up to 3 quantization indexes in the slice) if I well understood.
I would like to have something shorter too, but I preferred not to 
modify quant_table_index[i][j]

quant_table_count is the count of quant_tables
quant_table_index_count is the count of quant_table_index.

> please keep the line length of commit messages below 80
> but yes ive missed that part of the commit message

Ho, I was not aware of this limitation (I have carriage returns in my 
tools).
Modified to "50/72" best practice as seen in different places.

>
>> It is not good to have redundancy in a specification.
> Its more important that a spec is readable than being non redundant
> its after all a english text that has to be understood by a human

I kindly disagree: we need a good balance between redundancy and 
readability.
Here, I think the redundancy is not useful.
I plan to propose in the future a patch for a better readability of the 
order of planes.

> [...]
>

-------------- next part --------------
>From 9f4b73e4153b6c4829d814da15bf22695f1e3df9 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 14:57:27 +0200
Subject: [PATCH] Reduce redundancy in 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..e29f5e1 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 < primary_color_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 < primary_color_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
+primary_color_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