chromium-dev

[Chromium-dev] several bugs in pack/unpacking of textures

[Greg H. <hu...@gm...>]

Kevin Dale and I discovered several bugs yesterday in the pack/unpack
functions for the Compressed texture extensions.  There are 6 such
functions for 1,2,3D and full/sub textures.

First of all, packing functions are never responsible for including
the packet length themselves.  This is done implicitly by crPackAlloc.
 This causes the unpacker to get confused because the packet looks
like:

4 bytes: length put there by crPackAlloc
4 bytes: length (erroneously) put there by the packing function itself
4 bytes: extended opcode
4 bytes: texture target
...

etc

but the unpacker tries to interpret the second four bytes as the
extended opcode and dies.

Once we fixed this, we realized that the unpack versions of these
functions didn't exist.  I'm not sure why there are pack functions
without corresponding unpack functions.  They are very simple to
implement.

I fixed the packing functions, and wrote the unpacker for the one
function that Kevin was using.  Kevin, would you mind sending the list
the fixed pack_texture.c and unpack_texture.c so someone could check
them in (the changes are on his personal machine).

Also, it seems that we have been lax about surrounding things with
#ifdefs for extensions.  For example, we tried to undefine
CR_vertex_buffer_object, but this caused the state tracker to not
build because it unconditionally uses data structures that are
conditionally defined.

-- 
Greg Humphreys, Assistant Professor
Department of Computer Science, University of Virginia
http://www.cs.virginia.edu/~humper/

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Brian P. <bri...@tu...>]

Greg Humphreys wrote:
> Kevin Dale and I discovered several bugs yesterday in the pack/unpack
> functions for the Compressed texture extensions.  There are 6 such
> functions for 1,2,3D and full/sub textures.
> 
> First of all, packing functions are never responsible for including
> the packet length themselves.  This is done implicitly by crPackAlloc.
>  This causes the unpacker to get confused because the packet looks
> like:
> 
> 4 bytes: length put there by crPackAlloc
> 4 bytes: length (erroneously) put there by the packing function itself
> 4 bytes: extended opcode
> 4 bytes: texture target
> ...
> 
> etc
> 
> but the unpacker tries to interpret the second four bytes as the
> extended opcode and dies.
> 
> Once we fixed this, we realized that the unpack versions of these
> functions didn't exist.  I'm not sure why there are pack functions
> without corresponding unpack functions.  They are very simple to
> implement.

Probably just an oversight.

> I fixed the packing functions, and wrote the unpacker for the one
> function that Kevin was using.  Kevin, would you mind sending the list
> the fixed pack_texture.c and unpack_texture.c so someone could check
> them in (the changes are on his personal machine).
> 
> Also, it seems that we have been lax about surrounding things with
> #ifdefs for extensions.  For example, we tried to undefine
> CR_vertex_buffer_object, but this caused the state tracker to not
> build because it unconditionally uses data structures that are
> conditionally defined.

Yeah, there's probably lots of instances of that problem.  The 
original intention of those preprocessor symbols was to allow omitting 
extension code, but we've seldom actually tested that.

-Brian

RE: [Chromium-dev] several bugs in pack/unpacking of textures

[Kevin T D. <kd...@cs...>]

> -----Original Message-----
> From: chr...@li... [mailto:chromium-dev-
> ad...@li...] On Behalf Of Greg Humphreys
> Sent: Tuesday, April 05, 2005 9:03 AM
> To: chr...@li...; Kevin T Dale
> Subject: [Chromium-dev] several bugs in pack/unpacking of textures


> I fixed the packing functions, and wrote the unpacker for the one
> function that Kevin was using.  Kevin, would you mind sending the list
> the fixed pack_texture.c and unpack_texture.c so someone could check
> them in (the changes are on his personal machine).

Attached.

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Brian P. <bri...@tu...>]

Kevin T Dale wrote:
> 
>>-----Original Message-----
>>From: chr...@li... [mailto:chromium-dev-
>>ad...@li...] On Behalf Of Greg Humphreys
>>Sent: Tuesday, April 05, 2005 9:03 AM
>>To: chr...@li...; Kevin T Dale
>>Subject: [Chromium-dev] several bugs in pack/unpacking of textures
> 
> 
> 
>>I fixed the packing functions, and wrote the unpacker for the one
>>function that Kevin was using.  Kevin, would you mind sending the list
>>the fixed pack_texture.c and unpack_texture.c so someone could check
>>them in (the changes are on his personal machine).
> 
> 
> Attached.

I've checked in your changes, plus an update to unpacker_special.

It looks like the unpack routines for compressed 1D and 3D textures 
weren't updated though.

-Brian

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Greg H. <hu...@gm...>]

Yeah, I only did the one that he needed.  The other 5 are still unimplemented.

On Apr 6, 2005 2:29 PM, Brian Paul <bri...@tu...> wrote:
> Kevin T Dale wrote:
> >
> >>-----Original Message-----
> >>From: chr...@li... [mailto:chromium-dev-
> >>ad...@li...] On Behalf Of Greg Humphreys
> >>Sent: Tuesday, April 05, 2005 9:03 AM
> >>To: chr...@li...; Kevin T Dale
> >>Subject: [Chromium-dev] several bugs in pack/unpacking of textures
> >
> >
> >
> >>I fixed the packing functions, and wrote the unpacker for the one
> >>function that Kevin was using.  Kevin, would you mind sending the list
> >>the fixed pack_texture.c and unpack_texture.c so someone could check
> >>them in (the changes are on his personal machine).
> >
> >
> > Attached.
> 
> I've checked in your changes, plus an update to unpacker_special.
> 
> It looks like the unpack routines for compressed 1D and 3D textures
> weren't updated though.
> 
> -Brian
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Chromium-dev mailing list
> Chr...@li...
> https://lists.sourceforge.net/lists/listinfo/chromium-dev
> 
> 


-- 
Greg Humphreys, Assistant Professor
Department of Computer Science, University of Virginia
http://www.cs.virginia.edu/~humper/

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Brian P. <bri...@tu...>]

Greg Humphreys wrote:
> Yeah, I only did the one that he needed.  The other 5 are still unimplemented.

OK, I've added this as a TO-DO item so we don't totally forget about it.

-Brian

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Christopher W. <cr...@ms...>]

I just threw together the other 5.  I'm not checking it in myself 
because I'm only 99% sure it's right, mainly because I've never touched 
the unpacker before, but it was easy enough.  I also fixed the function 
that was just added (GLenum was used in all the READ_DATAs, instead of 
each variable's type)

ahh.. insomnia =)

 -Chris

Brian Paul wrote:

>
> Greg Humphreys wrote:
>
>> Yeah, I only did the one that he needed.  The other 5 are still 
>> unimplemented.
>
>
> OK, I've added this as a TO-DO item so we don't totally forget about it.
>
> -Brian
>
>
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Chromium-dev mailing list
> Chr...@li...
> https://lists.sourceforge.net/lists/listinfo/chromium-dev
>

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Brian P. <bri...@tu...>]

Christopher Waters wrote:
> I just threw together the other 5.  I'm not checking it in myself 
> because I'm only 99% sure it's right, mainly because I've never touched 
> the unpacker before, but it was easy enough.  I also fixed the function 
> that was just added (GLenum was used in all the READ_DATAs, instead of 
> each variable's type)

The code looks good to me.  I've checked it in.  Thanks!

-Brian

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Christopher W. <cr...@ms...>]

I went through a little more of the packer/unpacker code and tossed in a 
few more fixes. I also took out one of the "special case" functions, 
which seems to probably be the only one that 'needed' to be done at all.

I'm finding some quirks in that crPackFree is not being called in the 
functions that use crPackAlloc... shouldn't it always be called?

 -Chris

Brian Paul wrote:

> Christopher Waters wrote:
>
>> I just threw together the other 5.  I'm not checking it in myself 
>> because I'm only 99% sure it's right, mainly because I've never 
>> touched the unpacker before, but it was easy enough.  I also fixed 
>> the function that was just added (GLenum was used in all the 
>> READ_DATAs, instead of each variable's type)
>
>
> The code looks good to me.  I've checked it in.  Thanks!
>
> -Brian
>
>

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Greg H. <hu...@gm...>]

Well, you only need to call crPackFree if you are concerned about
leaking memory.

On Apr 10, 2005 7:44 AM, Christopher Waters <cr...@ms...> wrote:
> I went through a little more of the packer/unpacker code and tossed in a
> few more fixes. I also took out one of the "special case" functions,
> which seems to probably be the only one that 'needed' to be done at all.
> 
> I'm finding some quirks in that crPackFree is not being called in the
> functions that use crPackAlloc... shouldn't it always be called?
> 
>  -Chris
> 
> Brian Paul wrote:
> 
> > Christopher Waters wrote:
> >
> >> I just threw together the other 5.  I'm not checking it in myself
> >> because I'm only 99% sure it's right, mainly because I've never
> >> touched the unpacker before, but it was easy enough.  I also fixed
> >> the function that was just added (GLenum was used in all the
> >> READ_DATAs, instead of each variable's type)
> >
> >
> > The code looks good to me.  I've checked it in.  Thanks!
> >
> > -Brian
> >
> >
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Chromium-dev mailing list
> Chr...@li...
> https://lists.sourceforge.net/lists/listinfo/chromium-dev
> 
> 


-- 
Greg Humphreys, Assistant Professor
Department of Computer Science, University of Virginia
http://www.cs.virginia.edu/~humper/

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Brian P. <bri...@tu...>]

I just did a quick look into this, you should double-check before 
chaning anything.

Anyway, if you call crPackAlloc() you should use crHugePacket() send 
the message.  crHugePacket() calls the pc->SendHuge() callback.  In 
the case of the Pack SPU, this is the packspuHuge() function.  The 
last thing that function does is to call crPackFree() to deallocate 
the buffer.

You might check that the packer functions which call crPackAlloc() 
also call crHugePacket().

-Brian



Christopher Waters wrote:
> I went through a little more of the packer/unpacker code and tossed in a 
> few more fixes. I also took out one of the "special case" functions, 
> which seems to probably be the only one that 'needed' to be done at all.
> 
> I'm finding some quirks in that crPackFree is not being called in the 
> functions that use crPackAlloc... shouldn't it always be called?
> 
> -Chris
> 
> Brian Paul wrote:
> 
>> Christopher Waters wrote:
>>
>>> I just threw together the other 5.  I'm not checking it in myself 
>>> because I'm only 99% sure it's right, mainly because I've never 
>>> touched the unpacker before, but it was easy enough.  I also fixed 
>>> the function that was just added (GLenum was used in all the 
>>> READ_DATAs, instead of each variable's type)
>>
>>
>>
>> The code looks good to me.  I've checked it in.  Thanks!
>>
>> -Brian
>>
>>
> 
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Chromium-dev mailing list
> Chr...@li...
> https://lists.sourceforge.net/lists/listinfo/chromium-dev
> 

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Greg H. <hu...@gm...>]

He already checked in the extra calls to crPackFree(), which means
they are probably being called twice and should be rolled back.  Note
that this bug will only manifest itself if the texture is larger than
the pack buffer size, since crPackFree() is a NOP if the packet fits
in the buffer.

We have to leave the free'ing up to the SPU because some SPU's want to
keep these pack buffers around for a long time for later analysis or
playback.  You can't assume that HugePacket() is a fire-and-forget
model.

On Apr 11, 2005 11:25 AM, Brian Paul <bri...@tu...> wrote:
> 
> I just did a quick look into this, you should double-check before
> chaning anything.
> 
> Anyway, if you call crPackAlloc() you should use crHugePacket() send
> the message.  crHugePacket() calls the pc->SendHuge() callback.  In
> the case of the Pack SPU, this is the packspuHuge() function.  The
> last thing that function does is to call crPackFree() to deallocate
> the buffer.
> 
> You might check that the packer functions which call crPackAlloc()
> also call crHugePacket().
> 
> -Brian
> 
> 
> Christopher Waters wrote:
> > I went through a little more of the packer/unpacker code and tossed in a
> > few more fixes. I also took out one of the "special case" functions,
> > which seems to probably be the only one that 'needed' to be done at all.
> >
> > I'm finding some quirks in that crPackFree is not being called in the
> > functions that use crPackAlloc... shouldn't it always be called?
> >
> > -Chris
> >
> > Brian Paul wrote:
> >
> >> Christopher Waters wrote:
> >>
> >>> I just threw together the other 5.  I'm not checking it in myself
> >>> because I'm only 99% sure it's right, mainly because I've never
> >>> touched the unpacker before, but it was easy enough.  I also fixed
> >>> the function that was just added (GLenum was used in all the
> >>> READ_DATAs, instead of each variable's type)
> >>
> >>
> >>
> >> The code looks good to me.  I've checked it in.  Thanks!
> >>
> >> -Brian
> >>
> >>
> >
> >
> > -------------------------------------------------------
> > SF email is sponsored by - The IT Product Guide
> > Read honest & candid reviews on hundreds of IT Products from real users.
> > Discover which products truly live up to the hype. Start reading now.
> > http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> > _______________________________________________
> > Chromium-dev mailing list
> > Chr...@li...
> > https://lists.sourceforge.net/lists/listinfo/chromium-dev
> >
> 
> -------------------------------------------------------
> SF email is sponsored by - The IT Product Guide
> Read honest & candid reviews on hundreds of IT Products from real users.
> Discover which products truly live up to the hype. Start reading now.
> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
> _______________________________________________
> Chromium-dev mailing list
> Chr...@li...
> https://lists.sourceforge.net/lists/listinfo/chromium-dev
> 
> 


-- 
Greg Humphreys, Assistant Professor
Department of Computer Science, University of Virginia
http://www.cs.virginia.edu/~humper/

Re: [Chromium-dev] several bugs in pack/unpacking of textures

[Christopher W. <cr...@ms...>]

I do apologize for jumping the gun there. At the same time I did that, 
though, I fixed some functions where the packet size was being 
implicitly added to the packed data after calling crPackAlloc, which 
sets that itself, as Greg pointed out.

Looking through what I added to swap_texture, I noticed that most of 
the SWAP functions in pack_swap_texture make calls to crPackFree after 
crHugePacket.  I don't see any significant differences in those 
functions other than swapping the bytes, so perhaps those functions 
should be looked into as well.

  -Chris

On Apr 11, 2005, at 12:07 PM, Greg Humphreys wrote:

> He already checked in the extra calls to crPackFree(), which means
> they are probably being called twice and should be rolled back.  Note
> that this bug will only manifest itself if the texture is larger than
> the pack buffer size, since crPackFree() is a NOP if the packet fits
> in the buffer.
>
> We have to leave the free'ing up to the SPU because some SPU's want to
> keep these pack buffers around for a long time for later analysis or
> playback.  You can't assume that HugePacket() is a fire-and-forget
> model.
>
> On Apr 11, 2005 11:25 AM, Brian Paul <bri...@tu...> 
> wrote:
>>
>> I just did a quick look into this, you should double-check before
>> chaning anything.
>>
>> Anyway, if you call crPackAlloc() you should use crHugePacket() send
>> the message.  crHugePacket() calls the pc->SendHuge() callback.  In
>> the case of the Pack SPU, this is the packspuHuge() function.  The
>> last thing that function does is to call crPackFree() to deallocate
>> the buffer.
>>
>> You might check that the packer functions which call crPackAlloc()
>> also call crHugePacket().
>>
>> -Brian
>>
>>
>> Christopher Waters wrote:
>>> I went through a little more of the packer/unpacker code and tossed 
>>> in a
>>> few more fixes. I also took out one of the "special case" functions,
>>> which seems to probably be the only one that 'needed' to be done at 
>>> all.
>>>
>>> I'm finding some quirks in that crPackFree is not being called in the
>>> functions that use crPackAlloc... shouldn't it always be called?
>>>
>>> -Chris
>>>
>>> Brian Paul wrote:
>>>
>>>> Christopher Waters wrote:
>>>>
>>>>> I just threw together the other 5.  I'm not checking it in myself
>>>>> because I'm only 99% sure it's right, mainly because I've never
>>>>> touched the unpacker before, but it was easy enough.  I also fixed
>>>>> the function that was just added (GLenum was used in all the
>>>>> READ_DATAs, instead of each variable's type)
>>>>
>>>>
>>>>
>>>> The code looks good to me.  I've checked it in.  Thanks!
>>>>
>>>> -Brian
>>>>
>>>>
>>>
>>>
>>> -------------------------------------------------------
>>> SF email is sponsored by - The IT Product Guide
>>> Read honest & candid reviews on hundreds of IT Products from real 
>>> users.
>>> Discover which products truly live up to the hype. Start reading now.
>>> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
>>> _______________________________________________
>>> Chromium-dev mailing list
>>> Chr...@li...
>>> https://lists.sourceforge.net/lists/listinfo/chromium-dev
>>>
>>
>> -------------------------------------------------------
>> SF email is sponsored by - The IT Product Guide
>> Read honest & candid reviews on hundreds of IT Products from real 
>> users.
>> Discover which products truly live up to the hype. Start reading now.
>> http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
>> _______________________________________________
>> Chromium-dev mailing list
>> Chr...@li...
>> https://lists.sourceforge.net/lists/listinfo/chromium-dev
>>
>>
>
>
> -- 
> Greg Humphreys, Assistant Professor
> Department of Computer Science, University of Virginia
> http://www.cs.virginia.edu/~humper/
>