[FFmpeg-devel] DVCPRO HD: request for review

Michael Niedermayer michaelni
Fri Aug 29 21:37:10 CEST 2008


On Fri, Aug 29, 2008 at 11:42:05AM -0700, Baptiste Coudurier wrote:
> Michael, Roman,
> 
> Roman V. Shaposhnik wrote:
> > On Fri, 2008-08-29 at 19:50 +0200, Michael Niedermayer wrote:
> >> 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, 
> > 
> > Please point to a single issue (except the table) which wasn't resolved.

if you seriously want to know, there are plenty, some examples (from
what you commited)
@@ -386,10 +405,17 @@ static inline void dv_decode_video_segme
             dc = get_sbits(&gb, 9);
             dct_mode = get_bits1(&gb);
             class1 = get_bits(&gb, 2);
-            mb->idct_put = s->idct_put[dct_mode && log2_blocksize==3];
-            mb->scan_table = s->dv_zigzag[dct_mode];
-            mb->factor_table = s->dv_idct_factor[class1 == 3][dct_mode]
-                [quant + dv_quant_offset[class1]];
+            if (DV_PROFILE_IS_HD(s->sys)) {
+                mb->idct_put = s->idct_put[0];
+                mb->scan_table = s->dv_zigzag[0];
+                mb->factor_table = s->dv100_idct_factor[((s->sys->height == 720)<<1)&(j < 4)][class1][quant];
+                is_field_mode[mb_index] |= !j && dct_mode;                                                   
+            } else {
+                mb->idct_put = s->idct_put[dct_mode && log2_blocksize==3];
+                mb->scan_table = s->dv_zigzag[dct_mode];
+                mb->factor_table = s->dv_idct_factor[class1 == 3][dct_mode]
+                    [quant + dv_quant_offset[class1]];
+            }
             dc = dc << 2;
             /* convert to unsigned because 128 is not added in the
                standard IDCT */

it seems you have missed that this contains a cosmetic change (reindent) that
should be seperate from functional changes


+      .audio_stride = 90,
+      .audio_min_samples = { 1580, 1452, 1053 }, /* for 48, 44.1 and 32Khz */
+      .audio_samples_dist = { 1600, 1602, 1602, 1602, 1602 }, /* per SMPTE-314M */
+      .audio_shuffle = dv_audio_shuffle525,

that surely could have been vertically aligned


+static const int dv_iweight_1080_c[64] = {
+    128, 16, 16, 17, 17, 17, 25, 25,
+    25, 25, 26, 25, 26, 25, 26, 26,
+    26, 27, 27, 26, 26, 42, 38, 40,
+    40, 40, 38, 42, 44, 43, 41, 41,
+    41, 41, 43, 44, 91, 91, 84, 84,
+    84, 91, 91, 96, 93, 86, 86, 93,
+    96, 197, 191, 177, 191, 197, 203, 197,
+    197, 203, 209, 219, 209, 232, 232, 246,
+};

same here


+        /* We work with 720p frames split in half. The odd half-frame (chan==2,3) is displaced :-( */
+        if (s->sys->height == 720 && ((s->buf[1]>>2)&0x3) == 0) {
+               mb_y -= (mb_y>17)?18:-72; /* shifting the Y coordinate down by 72/2 macro blocks */
+        }

((s->buf[1]>>2)&0x3) == 0
can be simplified to
(s->buf[1]&0xC) == 0


+        for(j = 0; j < 2; j++, y_ptr += y_stride) {
+            for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))
+                 if (s->sys->pix_fmt == PIX_FMT_YUV422P && s->sys->width == 720 && i)
+                     y_ptr -= (1<<log2_blocksize);
+                 else
+                     mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);
+        }

this can be optimized to

if (s->sys->pix_fmt == PIX_FMT_YUV422P && s->sys->width == 720)
    for(j = 0; j < 2; j++, y_ptr += y_stride) {
        mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);
        block += 128;
        mb+=2;
        y_ptr += (1<<log2_blocksize));
    }
else
    for(j = 0; j < 2; j++, y_ptr += y_stride)
        for (i=0; i<2; i++, block += 64, mb++, y_ptr += (1<<log2_blocksize))
            mb->idct_put(y_ptr, s->picture.linesize[0]<<is_field_mode[mb_index], block);

besides
i++, block += 64, mb++, y_ptr += (1<<log2_blocksize)
is really
    i++;
    block += 64;
    mb++;
    y_ptr += (1<<log2_blocksize);
and that is even when its all on one line still rather slow.

But note this is not a complete review, iam sure there is more ...


> > 
> >>> 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.
> > 
> > Don't pretend you always know better. The commitments I have have more
> > to do with me feeling a responsibility as a DV maintainer, than with
> > anything you've imagined.

I was guessing what these "commitments" are yes, it seems i was guessing
wrong, sorry.


> > If you think that NON having DVPRO HD in
> > FFmpeg is better for the project, or that you can implement it yourself
> > or whatever -- I'd be happy to stop being part of this project *right
> > now* since I'm quite fed up with you personally and with how you've
> > managed to ruin the social microcosm around here.
> 
> Having DVCPRO HD in FFmpeg in definitely better, and yes Im personally
> interested in it, and I wrote the mov muxing code.
> 
> Now about the social microcosm running, I think honnestly this was true
> some time ago (especially when I was personally involved in fights), but
> I now really think that climate and microcosm changed lately, atmosphere
> is really better now IMHO.

Iam very happy to hear this, i was really affraid this thread would
degenerate into a "michael is evil" thread again ...


> 
> We all work for the sake of bringing FFmpeg up here, and we all have the
> same goal, we also have differents commitments and interests, discussing
> as a "team" is needed, not necessarly in a "pyramid" way (Michael /
> single dev people), however Michael tends to do everything around here,
> so this is less obvious IMHO.
> 
> Encouraging people working is really nice, and I try to do it as much as
> I can, and several people are doing it too.
> 
> <congrats>
> 
> I really liked Peter work on pcm audio, and I liked your work, Roman, on
> DVCPROHD, and I really liked Michael's H.264 work, also Stefano's work
> on documentation, Robert put many efforts in AAC decoder, and it was
> worth, and I'd also thank him for this awesome work, same for Kostya's
> work which will bring FFmpeg to another level without extra dependencies.
> 
> </congrats>

And id like also fix the remaining h.264 bugs, its just not easy between
all the patches and bugs and mails i have to deal with ...
I hope that as SOC ends ill be able to spend more time on it ...


[...]
> > Besides, speaking of unlcean code -- lets have an experiment on the
> > mailing list. The code is now in SVN (not for long, though) so is
> > there a single developer on this list who has a major issue 
> > with what has been committed (again, with a sole exception of tables
> > which I WILL resolve given a chance)? You keep implying a peer review,
> > so lets have a peer review.
> > 
> >> Where exactly did you do something on roundup? I cannot find a single mail
> >> from you on the corresponding mailing list.
> > 
> > I started working on two issues if you must know. Yes I'm slow and 
> > stupid, but if you are so smart and fats why do we still have 200+
> > bugs logged there?
> 
> Sorry but this is irrelevant, I can myself fix some bugs, and I do when
> I can, others can do too, and as a policy, I'd really like maintainers
> to fix bugs in their code, the whole team should try to fix all bugs in
> roundup.

i agree, but there are many bugs that are non trivial to fix, and especially
if taken litterally i do not think i could fix all bugs in code i maintain.
Some of these (like 4xm) are reverse engeneered formats where fixing a not
correctly decoded file can be very hard.


[...]
> Now I honestly really like you both, and I hope we all can settle these
> problems for the sake of FFmpeg project.

/me invites roman to a virtual glass of vodka to discuss what we shall
do now with the 170k in svn.

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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/664557e9/attachment.pgp>



More information about the ffmpeg-devel mailing list