[FFmpeg-devel] DVCPRO HD: request for review

Michael Niedermayer michaelni
Fri Aug 29 19:50:47 CEST 2008


On Fri, Aug 29, 2008 at 09:54:25AM -0700, Roman V. Shaposhnik wrote:
> On Fri, 2008-08-29 at 05:52 +0200, Michael Niedermayer wrote:
> > > I'll commit everything today and this piece
> > 
> > Iam not sure if this and your commit of 170k of unapproved code is a joke.
> > If it is, i do not like you humor.
> 
> No it isn't. It is an honest attempt to continue as a DV maintainer.
> 
> > Please revert it and commit the parts that have been approved cleanly
> > in several seperate commits.
> 
> No. And if treating maintainers in this manner is your kind of humor
> I don't think I appreciate it either. Moreover, this is not the first
> time when you ridicule me after I honestly tried to work all the issues
> with you and had an impression that they were all resolved.

They clearly where not resolved, and i do not belive that you seriously
belived that they where. I even told you per private mail that you should
commit the ok-ed parts several times that implicates that there are also
not ok-ed parts.


> I do have
> commitments as far as DVCPRO HD is concerned 

I could have guessed that much from the daily private mails from you about
reviewing your patch soon and quickly. And that you would help me where
possible if i just review them quickly.
I told you many things where you could help, like commiting the parts of
the patch ive ok-ed or comiting some of the ok-ed parts of patches of
SOC students or fixing some bugs at roundup. You did _nothing_
you where even too lazy to commit the parts of your OWN patch that i
had approved you just reposted the whole and kept pushing me to review it
again
sorry but in this case "your commitments" seems to be quite a bit in
the way of keeping the code in reassonable clean shape.


> so I'm willing to tolerate
> your behavior while I'm trying to integrate decoder and encoder. 
> But once that happens, I believe I have no place in the project like
> this.

That is sad, but i will not tolerate that people start commiting unclean
code because their payment from some contact might depend on it.


> So, please start finding another maintainer for DV stuff. Heavens
> are my witness I tried to work with you. I tried to be helpful to the
> project. I'm not as smart as you are. But I believe I can be helpful.
> You know that I went over the roundup  yesterday and saw a couple of 
> areas where my puny coding skills are a match. But you, know, I really 
> don't need this aggravation...

Where exactly did you do something on roundup? I cannot find a single mail
from you on the corresponding mailing list.


> 
> Anyway, now for the technical part...
> 
> Yes. I will revert the commit once I get to a place where svn works.
> This likely to happen by COB today. However, I have no clue what you're
> talking about when you say that I have committed unapproved code. As
> far as I'm concerned everything in:
>     libavformat/isom.c has been approved
>     libavformat/dv.c has been approved
>     libavcodec/dv.c has been approved
> the only bit you didn't *explicitly* approved is the tables. And given
> that I explained two times why I am adding them and why they have
> a high chance of going away real soon once I transition to any
> scheme that proves to be faster. It is all documented in the 
> thread -- look it up.

Well, so the tables are in svn now, what holds you up with replacing them
now as you promissed in the thread?


> 
> Now, as for committing individual changes. There's none to be committed
> (except for isom.c) so that you have a *working* DV codec between the
> commits. I have always though that people are using changesets for a

Some examples:

@@ -59,8 +64,8 @@ typedef struct DVVideoContext {

 /* MultiThreading - dv_anchor applies to entire DV codec, not just the avcontext */
 /* one element is needed for each video segment in a DV frame */
-/* at most there are 2 DIF channels * 12 DIF sequences * 27 video segments (PAL 50Mbps) */
-#define DV_ANCHOR_SIZE (2*12*27)
+/* at most there are 4 DIF channels * 12 DIF sequences * 27 video segments (1080i50) */
+#define DV_ANCHOR_SIZE (4*12*27)

 static void* dv_anchor[DV_ANCHOR_SIZE];
------------------
@@ -51,6 +55,7 @@ typedef struct DVVideoContext {

     uint8_t dv_zigzag[2][64];
     uint32_t dv_idct_factor[2][2][22][64];
+    uint32_t dv100_idct_factor[4][4][16][64];

     void (*get_pixels)(DCTELEM *block, const uint8_t *pixels, int line_size);
     void (*fdct[2])(DCTELEM *block);
@@ -102,6 +107,17 @@ static void dv_build_unquantize_tables(D
             }
         }
     }
+
+    for(a = 0; a < 4; a++) {
+        for(q = 0; q < 16; q++) {
+            for(i = 1; i < 64; i++) {
+                s->dv100_idct_factor[0][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_y[i];
+                s->dv100_idct_factor[1][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_1080_c[i];
+                s->dv100_idct_factor[2][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_y[i];
+                s->dv100_idct_factor[3][a][q][i]= (dv100_qstep[q]<<(a+9))*dv_iweight_720_c[i];
+            }
+        }
+    }
 }

 static av_cold int dvvideo_init(AVCodecContext *avctx)
Modified: trunk/libavcodec/dvdata.h
==============================================================================
--- trunk/libavcodec/dvdata.h   (original)
+++ trunk/libavcodec/dvdata.h   Fri Aug 29 00:41:00 2008
@@ -316,6 +316,13 @@ static const uint8_t dv_quant_shifts[22]
 static const uint8_t dv_quant_offset[4] = { 6, 3, 0, 1 };
 static const uint8_t dv_quant_areas[4] = { 6, 21, 43, 64 };

+/* quantization quanta by QNO for DV100 */
+static const uint8_t dv100_qstep[16] = {
+    1, /* QNO = 0 and 1 both have no quantization */
+    1,
+    2, 3, 4, 5, 6, 7, 8, 16, 18, 20, 22, 24, 28, 52
+};
+
 /* NOTE: I prefer hardcoding the positioning of dv blocks, it is
    simpler :-) */

----------------------
@@ -391,6 +410,11 @@ static int dv_read_header(AVFormatContex
         return AVERROR(EIO);

     c->dv_demux->sys = dv_frame_profile(c->buf);
+    if (!c->dv_demux->sys) {
+        av_log(s, AV_LOG_ERROR, "Can't determine profile of DV input stream.\n");
+        return -1;
+    }
+
     s->bit_rate = av_rescale(c->dv_demux->sys->frame_size * 8,
                              c->dv_demux->sys->frame_rate,
                              c->dv_demux->sys->frame_rate_base);

-----------------------
And of course there are many more, but iam not too interrested in spliting
the 170k patch for you


[...]

> > Several people including me and vitor try hard to cleanup codecs that had
> > been added without passing through proper review, we really do not need
> > more such code.
> 
> What code?

the 170k you commited, i have no interrest in cleaning it up.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080829/bca22bb6/attachment.pgp>



More information about the ffmpeg-devel mailing list