[FFmpeg-devel] [PATCH] H264 DXVA2 implementation

Michael Niedermayer michaelni
Fri Jan 8 15:47:05 CET 2010


On Fri, Jan 08, 2010 at 07:54:53AM +0100, Reimar D?ffinger wrote:
> On Thu, Jan 07, 2010 at 11:19:26PM +0100, Laurent Aimar wrote:
[...]
> > > > +    if (h->sps.profile_idc >= 100) {
> > > > +        pp->bit_depth_luma_minus8     = h->sps.bit_depth_luma - 8;
> > > > +        pp->bit_depth_chroma_minus8   = h->sps.bit_depth_chroma - 8;
> > > > +    } else {
> > > > +        pp->bit_depth_luma_minus8     = 0;
> > > > +        pp->bit_depth_chroma_minus8   = 0;
> > > > +    }
> > > Again, why not in sps reading code (the condition, not the - 8)?
> >  For that one (where a sensible value stored by h264.c would also match the
> > requirement of DXVA2), I would say that it doesn't seem the spirit of h264.c
> > to initialized non bitstream defined value with a valid one.
> 
> I can't tell, but we definitely should ask the maintainer (Michael I think?),
> because this is quite ugly and if we can we should avoid it.

h->sps.bit_depth* like other fields should reflect the truth, if they are
explicitly stored or not.
Now it seems that variable is never used in our decoder and also not really
initialized in that sense. but if it where used we surely would use it in a
way like
if(bit_depth_luma > 8)
and not like
if(profile_idc >= 100 && bit_depth_luma > 8)


> 
> > > > +    for (i = 0; i < 6; i++)
> > > > +        for (j = 0; j < 16; j++)
> > > > +            qm->bScalingLists4x4[i][j] = h->pps.scaling_matrix4[i][zigzag_scan[j]];
> > > > +
> > > > +    for (i = 0; i < 2; i++)
> > > > +        for (j = 0; j < 64; j++)
> > > > +            qm->bScalingLists8x8[i][j] = h->pps.scaling_matrix8[i][ff_zigzag_direct[j]];
> > > 
> > > Swap the loop, and in the second case unroll it.
> > > I doubt speed matters, but then you can do the zigzag lookup in the outer loop
> > > which is more readable IMO.
> > 
> > for (j = 0; j < 16; j++) {
> >     const int zj = zigzag_scan[j];
> >     for (i = 0; i < 6; i++)
> >         qm->bScalingLists4x4[i][j] = h->pps.scaling_matrix4[i][zj];
> > }
> > for (j = 0; j < 64; j++) {
> >     const int zj = ff_zigzag_direct[j];
> >     qm->bScalingLists8x8[i][j] = h->pps.scaling_matrix8[0][zj];
> >     qm->bScalingLists8x8[i][j] = h->pps.scaling_matrix8[1][zj];
> > }
> > 
> > Like that ?
> > Not sure it helps/improves (and no, speed does not matter at all here).
> 
> Well, you forgot to replace two i in the second case, and yes I definitely
> think it is an improvement there.
> Not so sure about the first one, but IMO it isn't worse either.

i dunno, i see no problem in the original


[...]
> > > > +#define OFFSET_OF(t,f) ((intptr_t)&((t*)NULL)->f)
> > > > +            assert(OFFSET_OF(DXVA_Slice_H264_Short, BSNALunitDataLocation) == 
> > > > +                   OFFSET_OF(DXVA_Slice_H264_Long,  BSNALunitDataLocation));
> > > > +            assert(OFFSET_OF(DXVA_Slice_H264_Short, SliceBytesInBuffer) == 
> > > > +                   OFFSET_OF(DXVA_Slice_H264_Long,  SliceBytesInBuffer));
> > > > +#undef OFFSET_OF
> > > 
> > > There is offsetof which probably works at least as well as this hack which
> > > invokes undefined behaviour.
> > Why does it invoke undefined behaviour ?
> 
> Because pointer arithmetic is only defined as long as the result is within
> an allocated memory range or at most one beyond it.
> Since NULL in general has no allocated memory associated with it, pointer
> arithmetic on it should always be undefined, and the same should apply
> to this kind of address calculation.

sadly ...


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- 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/20100108/c4bbd145/attachment.pgp>



More information about the ffmpeg-devel mailing list