IFF demuxer and 8SVX decoder

On Saturday 29 March 2008 14:46:01 Michael Niedermayer
wrote:
> On Sat, Mar 29, 2008 at 11:13:18AM +0000, Jai Menon
wrote:
> > On Friday 28 March 2008 18:29:35 Michael
Niedermayer wrote:
> > > On Fri, Mar 28, 2008 at 09:21:36PM +0000, Jai
Menon wrote:
> > > > On Thursday 27 March 2008 20:00:48
Michael Niedermayer wrote:
> > > > > On Thu, Mar 27, 2008 at 11:48:10PM
+0000, Jai Menon wrote:
> > > > > > On Thursday 27 March 2008
15:23:54 Michael Niedermayer wrote:
> > > > > > > On Thu, Mar 27, 2008 at
08:44:54PM +0000, Jai Menon wrote:
> > > > > > > > On Wednesday 26
March 2008 21:12:26 Michael Niedermayer wrote:
> > > > > > > > > uint8_t d =
*buf++;
> > > > > > > > >
> > > > > > > > > > +       
esc->fib_acc += esc->table[d & 0x0f];
> > > > > > > > > > +       
*out_data++ = esc->fib_acc << 8;
> > > > > > > > > > +       
esc->fib_acc += esc->table[d >> 4];
> > > > > > > > > > +       
*out_data++ = esc->fib_acc << 8;
> > > > > > > > > > +    }
> > > > > > > > >
> > > > > > > > > you can do this
with one subtraction and 2 shifts less
> > > > > > > >
> > > > > > > > I still don't know
how i can eliminate the two shifts?
> > > > > > >
> > > > > > > change the table ...
> > > > > >
> > > > > > I could change it to int16_t,
and remove the 2 shifts.....but
> > > > > > then i would need to clip
twice before adding the table value to
> > > > > > the accumulator......in which
case imho we should stick to 2
> > > > > > shifts.
> > > > >
> > > > > Why would you need to clip?
> > > >
> > > >  Thats because the encoding scheme
requires adding an 8 bit signed
> > > > value (from the table) to fib_acc. So if
we change the table size to
> > > > int16_t , we could do away with the
shifts but the value can't be
> > > > directly added to fib_acc without
clipping.
> > >
> > > Could you give me an example with numbers
where a single value ending
> > > up in out_data differs?
> >
> > I'll phrase this in a different way. After the
decoding, the fib_acc
> > variable stores the signed pcm data. The shifts
are required because the
> > decoder's output is supposed to be 16 bit. If i
convert the table to use
> > 16 bit data instead and also make fib_acc an
int16_t, i'll need to
> > extract just the lsbyte since the msbyte is not
part of the sample
> > information.
> > Otherwise, i could use a couple of casts to ensure
that the msbyte
> > remains 0x00, but that wouldn't be too clean.
>
> Currently the code does practically:
>
> while(not end){
>     acc += constant_table[*x++];
>     *out++= acc<<8;
> }
>
> The code can be changed so it does not need the
"<<8" while keeping the
> output the same 16bit data.
I have removed the shifts. The changes should clarify some
stuff on why the 
previous code was like that.

>
> > +#include <stdio.h>
> > +#include <stdlib.h>
>
> are all these includes needed?
These must have been there in the existing codec/demuxer i
used as 
a "template". Sorry i didn't remove these
earlier.
 
whew!! day 7 rolls on.........

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sunday 30 March 2008 10:43:16 Jai Menon wrote:
> On Saturday 29 March 2008 14:46:01 Michael Niedermayer
wrote:
> > On Sat, Mar 29, 2008 at 11:13:18AM +0000, Jai
Menon wrote:
> > > On Friday 28 March 2008 18:29:35 Michael
Niedermayer wrote:
> > > > On Fri, Mar 28, 2008 at 09:21:36PM
+0000, Jai Menon wrote:
> > > > > On Thursday 27 March 2008 20:00:48
Michael Niedermayer wrote:
> > > > > > On Thu, Mar 27, 2008 at
11:48:10PM +0000, Jai Menon wrote:
> > > > > > > On Thursday 27 March 2008
15:23:54 Michael Niedermayer wrote:
> > > > > > > > On Thu, Mar 27, 2008
at 08:44:54PM +0000, Jai Menon wrote:
> > > > > > > > > On Wednesday 26
March 2008 21:12:26 Michael Niedermayer 
wrote:
> > > > > > > > > > uint8_t d
= *buf++;
> > > > > > > > > >
> > > > > > > > > > > +    
   esc->fib_acc += esc->table[d & 0x0f];
> > > > > > > > > > > +    
   *out_data++ = esc->fib_acc << 8;
> > > > > > > > > > > +    
   esc->fib_acc += esc->table[d >> 4];
> > > > > > > > > > > +    
   *out_data++ = esc->fib_acc << 8;
> > > > > > > > > > > +   
}
> > > > > > > > > >
> > > > > > > > > > you can do
this with one subtraction and 2 shifts less
> > > > > > > > >
> > > > > > > > > I still don't
know how i can eliminate the two shifts?
> > > > > > > >
> > > > > > > > change the table
...
> > > > > > >
> > > > > > > I could change it to
int16_t, and remove the 2 shifts.....but
> > > > > > > then i would need to clip
twice before adding the table value
> > > > > > > to the
accumulator......in which case imho we should stick to 2
> > > > > > > shifts.
> > > > > >
> > > > > > Why would you need to clip?
> > > > >
> > > > >  Thats because the encoding scheme
requires adding an 8 bit signed
> > > > > value (from the table) to fib_acc.
So if we change the table size
> > > > > to int16_t , we could do away with
the shifts but the value can't
> > > > > be directly added to fib_acc
without clipping.
> > > >
> > > > Could you give me an example with
numbers where a single value ending
> > > > up in out_data differs?
> > >
> > > I'll phrase this in a different way. After
the decoding, the fib_acc
> > > variable stores the signed pcm data. The
shifts are required because
> > > the decoder's output is supposed to be 16
bit. If i convert the table
> > > to use 16 bit data instead and also make
fib_acc an int16_t, i'll need
> > > to extract just the lsbyte since the msbyte
is not part of the sample
> > > information.
> > > Otherwise, i could use a couple of casts to
ensure that the msbyte
> > > remains 0x00, but that wouldn't be too
clean.
> >
> > Currently the code does practically:
> >
> > while(not end){
> >     acc += constant_table[*x++];
> >     *out++= acc<<8;
> > }
> >
> > The code can be changed so it does not need the
"<<8" while keeping the
> > output the same 16bit data.
>
> I have removed the shifts. The changes should clarify
some stuff on why the
> previous code was like that.
>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> >
> > are all these includes needed?
>
> These must have been there in the existing
codec/demuxer i used as
> a "template". Sorry i didn't remove these
earlier.
>
> whew!! day 7 rolls on.........

sorry.......the demuxer patch is an old one. new one
attached below

Regards
Jai Menon
<realitymangmx.net>


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sun, 2008-03-30 at 10:43 +0000, Jai Menon wrote:
> On Saturday 29 March 2008 14:46:01 Michael Niedermayer
wrote:
> > Currently the code does practically:
> >
> > while(not end){
> >     acc += constant_table[*x++];
> >     *out++= acc<<8;
> > }
> >
> > The code can be changed so it does not need the
"<<8" while keeping the
> > output the same 16bit data.
> I have removed the shifts. The changes should clarify
some stuff on why the 
> previous code was like that.

But you added a bswap in the loop instead of changing the
table. I don't
think that is what was intended...

Btw the timestamp of your mail is completely wrong, it's
hours into the
future.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sunday 30 March 2008 05:40:03 Uoti Urpala wrote:
> On Sun, 2008-03-30 at 10:43 +0000, Jai Menon wrote:
> > On Saturday 29 March 2008 14:46:01 Michael
Niedermayer wrote:
> > > Currently the code does practically:
> > >
> > > while(not end){
> > >     acc += constant_table[*x++];
> > >     *out++= acc<<8;
> > > }
> > >
> > > The code can be changed so it does not need
the "<<8" while keeping the
> > > output the same 16bit data.
> >
> > I have removed the shifts. The changes should
clarify some stuff on why
> > the previous code was like that.
>
> But you added a bswap in the loop instead of changing
the table. I don't
> think that is what was intended...
 
exactly....neither do i !!! i just wanted to write the
decoding step in a 
different way to show why i prefer the previous method. But
i'm still 
experimenting .......
> Btw the timestamp of your mail is completely wrong,
it's hours into the
> future.

 Could be my timezone ? any ideas on how to "fix"
this...........
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sunday 30 March 2008 11:26:16 Jai Menon wrote:
> On Sunday 30 March 2008 05:40:03 Uoti Urpala wrote:
> > On Sun, 2008-03-30 at 10:43 +0000, Jai Menon
wrote:
> > > On Saturday 29 March 2008 14:46:01 Michael
Niedermayer wrote:
> > > > Currently the code does practically:
> > > >
> > > > while(not end){
> > > >     acc += constant_table[*x++];
> > > >     *out++= acc<<8;
> > > > }
> > > >
> > > > The code can be changed so it does not
need the "<<8" while keeping
> > > > the output the same 16bit data.
> > >
> > > I have removed the shifts. The changes should
clarify some stuff on why
> > > the previous code was like that.
> >
> > But you added a bswap in the loop instead of
changing the table. I don't
> > think that is what was intended...
>
> exactly....neither do i !!! i just wanted to write the
decoding step in a
> different way to show why i prefer the previous method.
But i'm still
> experimenting .......

Another interpretation........

Regards
Jai Menon
<realitymangmx.net>



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sun, Mar 30, 2008 at 11:43:32AM +0000, Jai Menon wrote:
> On Sunday 30 March 2008 11:26:16 Jai Menon wrote:
> > On Sunday 30 March 2008 05:40:03 Uoti Urpala
wrote:
> > > On Sun, 2008-03-30 at 10:43 +0000, Jai Menon
wrote:
> > > > On Saturday 29 March 2008 14:46:01
Michael Niedermayer wrote:
> > > > > Currently the code does
practically:
> > > > >
> > > > > while(not end){
> > > > >     acc += constant_table[*x++];
> > > > >     *out++= acc<<8;
> > > > > }
> > > > >
> > > > > The code can be changed so it does
not need the "<<8" while keeping
> > > > > the output the same 16bit data.
> > > >
> > > > I have removed the shifts. The changes
should clarify some stuff on why
> > > > the previous code was like that.
> > >
> > > But you added a bswap in the loop instead of
changing the table. I don't
> > > think that is what was intended...
> >
> > exactly....neither do i !!! i just wanted to write
the decoding step in a
> > different way to show why i prefer the previous
method. But i'm still
> > experimenting .......
> 
> Another interpretation........

[...]
> +const static int16_t fibonacci[16]   = { -8704, -5376,
-3328, -2048, -1280, -768, -512, -256,
> +                                        0, 256, 512,
768, 1280, 2048, 3328, 5376 };
> +const static int16_t exponential[16] = { -32768,
-16384, -8192, -4096, -2048, -1024, -512, -256,
> +                                        0, 256, 512,
1024, 2048, 4096, 8192, 16384};

These could be written like:
const static int16_t fibonacci[16]   = {-34<<8,
-21<<8, -13<<8, ...

This is no problem as the compiler does these shifts during
compilation, thus
no unneeded code nor time is wasted during execution.


> +
> +/**
> + * decode a frame
> + */
> +static int eightsvx_decode_frame(AVCodecContext
*avctx, void *data, int *data_size,
> +                                 const uint8_t *buf,
int buf_size)
> +{
> +    EightSvxContext *esc = avctx->priv_data;
> +    int16_t *out_data = data;
> +    int consumed = buf_size;
> +    const uint8_t *buf_end = buf + buf_size;
> +
> +    if((*data_size >> 2) < buf_size)
> +        return -1;
> +
> +    if(avctx->frame_number == 0) {

> +        esc->fib_acc = bswap_16(buf[1]);

That can use a shift, my goal was to remove unneeded
operations in the more
speed critical parts of the code, not to remove shifts
"just because of it".


> +        buf_size -= 2;
> +        buf += 2;
> +    }
> +
> +    *data_size = buf_size << 2;
> +

> +    while(buf < buf_end) {
> +        uint8_t d = *buf++;
> +        esc->fib_acc += esc->table[d &
0x0f];
> +        *out_data++ = esc->fib_acc;
> +        esc->fib_acc += esc->table[d >>
4];
> +        *out_data++ = esc->fib_acc;
> +    }

good!

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

If a bugfix only changes things apparently unrelated to the
bug with no
further explanation, that is a good sign that the bugfix is
wrong.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Hi
required changes were made in the decoder.
Can this be commited now?

Regards
Jai Menon
<realitymangmx.net>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sun, Mar 30, 2008 at 03:29:41PM +0000, Jai Menon wrote:
> Hi
> required changes were made in the decoder.
> Can this be commited now?

ok

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

The educated differ from the uneducated as much as the
living from the
dead. -- Aristotle 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sun, Mar 30, 2008 at 11:00:16AM +0000, Jai Menon wrote:
> On Sunday 30 March 2008 10:43:16 Jai Menon wrote:
> > On Saturday 29 March 2008 14:46:01 Michael
Niedermayer wrote:
> > > On Sat, Mar 29, 2008 at 11:13:18AM +0000, Jai
Menon wrote:
> > > > On Friday 28 March 2008 18:29:35 Michael
Niedermayer wrote:
> > > > > On Fri, Mar 28, 2008 at 09:21:36PM
+0000, Jai Menon wrote:
> > > > > > On Thursday 27 March 2008
20:00:48 Michael Niedermayer wrote:
> > > > > > > On Thu, Mar 27, 2008 at
11:48:10PM +0000, Jai Menon wrote:
> > > > > > > > On Thursday 27 March
2008 15:23:54 Michael Niedermayer wrote:
> > > > > > > > > On Thu, Mar 27,
2008 at 08:44:54PM +0000, Jai Menon wrote:
> > > > > > > > > > On
Wednesday 26 March 2008 21:12:26 Michael Niedermayer 
> wrote:
> > > > > > > > > > >
uint8_t d = *buf++;
> > > > > > > > > > >
> > > > > > > > > > > >
+        esc->fib_acc += esc->table[d & 0x0f];
> > > > > > > > > > > >
+        *out_data++ = esc->fib_acc << 8;
> > > > > > > > > > > >
+        esc->fib_acc += esc->table[d >> 4];
> > > > > > > > > > > >
+        *out_data++ = esc->fib_acc << 8;
> > > > > > > > > > > >
+    }
> > > > > > > > > > >
> > > > > > > > > > > you
can do this with one subtraction and 2 shifts less
> > > > > > > > > >
> > > > > > > > > > I still
don't know how i can eliminate the two shifts?
> > > > > > > > >
> > > > > > > > > change the
table ...
> > > > > > > >
> > > > > > > > I could change it to
int16_t, and remove the 2 shifts.....but
> > > > > > > > then i would need to
clip twice before adding the table value
> > > > > > > > to the
accumulator......in which case imho we should stick to 2
> > > > > > > > shifts.
> > > > > > >
> > > > > > > Why would you need to
clip?
> > > > > >
> > > > > >  Thats because the encoding
scheme requires adding an 8 bit signed
> > > > > > value (from the table) to
fib_acc. So if we change the table size
> > > > > > to int16_t , we could do away
with the shifts but the value can't
> > > > > > be directly added to fib_acc
without clipping.
> > > > >
> > > > > Could you give me an example with
numbers where a single value ending
> > > > > up in out_data differs?
> > > >
> > > > I'll phrase this in a different way.
After the decoding, the fib_acc
> > > > variable stores the signed pcm data. The
shifts are required because
> > > > the decoder's output is supposed to be
16 bit. If i convert the table
> > > > to use 16 bit data instead and also make
fib_acc an int16_t, i'll need
> > > > to extract just the lsbyte since the
msbyte is not part of the sample
> > > > information.
> > > > Otherwise, i could use a couple of casts
to ensure that the msbyte
> > > > remains 0x00, but that wouldn't be too
clean.
> > >
> > > Currently the code does practically:
> > >
> > > while(not end){
> > >     acc += constant_table[*x++];
> > >     *out++= acc<<8;
> > > }
> > >
> > > The code can be changed so it does not need
the "<<8" while keeping the
> > > output the same 16bit data.
> >
> > I have removed the shifts. The changes should
clarify some stuff on why the
> > previous code was like that.
> >
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > >
> > > are all these includes needed?
> >
> > These must have been there in the existing
codec/demuxer i used as
> > a "template". Sorry i didn't remove
these earlier.
> >
> > whew!! day 7 rolls on.........
> 
> sorry.......the demuxer patch is an old one. new one
attached below

[...]
> +static int iff_probe(AVProbeData *p)
> +{
> +    const uint8_t *d = p->buf;
> +
> +    if (AV_RL32(d) == ID_FORM
> +        && AV_RL32(d+8) == ID_8SVX)

if (   AV_RL32(d  ) == ID_FORM
    && AV_RL32(d+8) == ID_8SVX)

or
if (AV_RL32(d  ) == ID_FORM &&
    AV_RL32(d+8) == ID_8SVX)

look better IMHO


[...]

> +static int iff_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    IffDemuxContext *iff = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int ret;
> +
> +    if(iff->sent_bytes > iff->body_size)
> +        return AVERROR(EIO);
> +    ret = av_get_packet(pb, pkt, PACKET_SIZE);
> +    iff->sent_bytes += PACKET_SIZE;
> +
> +    pkt->stream_index = 0;
> +    pkt->pts = iff->audio_frame_count;
> +    iff->audio_frame_count += ret /
s->streams[0]->codec->channels;
> +    return ret;
> +}

the first packet should have PKT_FLAG_KEY


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

It is dangerous to be right in matters on which the
established authorities
are wrong. -- Voltaire

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sunday 30 March 2008 11:19:23 Michael Niedermayer wrote:
> > +static int iff_probe(AVProbeData *p)
> > +{
> > +    const uint8_t *d = p->buf;
> > +
> > +    if (AV_RL32(d) == ID_FORM
> > +        && AV_RL32(d+8) == ID_8SVX)
>
> if (   AV_RL32(d  ) == ID_FORM
>     && AV_RL32(d+8) == ID_8SVX)
>
> or
> if (AV_RL32(d  ) == ID_FORM &&
>     AV_RL32(d+8) == ID_8SVX)
>
> look better IMHO

Done

> > +static int iff_read_packet(AVFormatContext *s,
> > +                           AVPacket *pkt)
> > +{
> > +    IffDemuxContext *iff = s->priv_data;
> > +    ByteIOContext *pb = s->pb;
> > +    int ret;
> > +
> > +    if(iff->sent_bytes >
iff->body_size)
> > +        return AVERROR(EIO);
> > +    ret = av_get_packet(pb, pkt, PACKET_SIZE);
> > +    iff->sent_bytes += PACKET_SIZE;
> > +
> > +    pkt->stream_index = 0;
> > +    pkt->pts = iff->audio_frame_count;
> > +    iff->audio_frame_count += ret /
s->streams[0]->codec->channels;
> > +    return ret;
> > +}
>
> the first packet should have PKT_FLAG_KEY
Done


Can i commit to svn now??


Regards
Jai Menon
<realitymangmx.net>

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel