[FFmpeg-soc] [Patch] Maxis EA XA decoder - GSoC Task

[Michael Niedermayer]

On Sat, Apr 12, 2008 at 01:39:45PM +0200, Robert Marston wrote:
> On Fri, Apr 11, 2008 at 8:18 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Apr 11, 2008 at 04:19:37PM +0000, compn wrote:
> >  > Michael Niedermayer <michaelni at ...> writes:
> >  >
> >  > >
> >  > > On Fri, Apr 11, 2008 at 03:41:48PM +0200, Robert Marston wrote:
> >  > > > >  > > > +                    sample = ((((*(src+channel) >> i) & 0x0F) <<
> >  > 0x1C) >> shift[channel]);
> >  > > > >  > >
> >  > > > >  > >  This looks buggy.
> >  > > > >  >
> >  > > > >  > Where do you think the error would occur?
> >  > > > >
> >  > > > >  on some non x86 hardware
> >  > > >
> >  > > > What exactly you referring to here? The shift operators? A problem
> >  > > > with the Endianess maybe?
> >  > >
> >  > > Its related to the shifts.
> >  >
> >  >
> >  > i dont mean to be rude, but i believe that Robert may need some more verbose
> >  > output from you :)
> >
> >  Good C knowledge is a requirement for ffmpeg gsoc.
> >  If one uses a strict interpretation of the C standard then there are multiple
> >  bugs in the line above. Iam not doing that, i just mean one actual issue
> >  which can occur on real hardware.
> >
> >  Heres an example to help a little
> >  *(src+channel)= 234
> >  i= 4
> >  shift[channel]= 20
> >  sample=  -512 (normlly)
> >  sample=  3584 (on ILP64)
> >
> 
> Thanks for pointing that out, I will be the first to admit that my c
> knowledge is probably not up to standard when it comes to FFMPEG and
> as such would required much feedback from my mentor. I see the GSoC as
> good learning opportunity for the myself and a chance to bolster open
> source development and potential to increase the code base of the
> mentor organizations project.
> 
> Would casting the *(src + channel) to a int32_t stop the above from happening?

i would put the cast after <<0x1C but before >>shift[channel]


> 
> >
> >  >
> >  > > > >  > >  Still wrong
> >  > > > >  > Corrected?
> >  > > > >  no
> >  > > > Am I right in saying the pts should be incremented by 28 *
> >  > > no
> >  >
> >  >
> >  > more verbosity needed here too.
> >
> >  Iam not sure what verbosity over
> >
> >  typedef struct AVPacket {
> >     int64_t pts;                            ///< presentation time stamp in time_base units
> >
> >  is needed? Also there is the source of other demuxers and the source of
> >  the code using pts.
> >  Robert has an application for H.264 SVC (H.264 is one of the most complex
> >  codecs around) how is he planing to understand the existing code and
> >  integrate SVC support if he cannot figure out how to set the timestamps
> >  for an adpcm codec?
> >
> >  [...]
> 
> My logic on this is that there are 2 samples per byte and 14 bytes per
> channel. = 28 x num_channels
> There are is 1 sample every 1 / sample rate seconds and 90 K ticks per
> second if we use a 90 KHz clock
> 
> Therefore the pts, which in my understanding of the time base being
> the number of ticks of the 90 KHz clock, will be advanced  by 28 x
> num_channels /  sample rate * 90 K for each block read from the file.
> 
> Have I misunderstood something here?

yes, there is no 90khz clock. The "clock" is what the code calls time_base
and what you set with av_set_pts_info()

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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-soc/attachments/20080412/eeab7292/attachment.pgp>