[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