Closed Bug 449028 Opened 16 years ago Closed 16 years ago

libtheora's cpu.c assumes x86

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: blassey, Assigned: dougt)

Details

(Keywords: mobile)

Attachments

(1 file, 3 obsolete files)

patch v.4
1.37 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Attachment #332208 - Flags: review?(roc)
That doesn't make sense since we'll still barf on non-ARM non-x86. I think USE_ASM should only be defined if we're actually on x86 (which is tested just above).
Yes, it should be:

ifeq ($(findstring 86,$(OS_TEST)), 86)
DEFINES += -DOC_X86ASM -DUSE_ASM
endif

And remove the existing check for Linux and defining it.
Attached patch patch v.2 (obsolete) — — Splinter Review 
what chris said.
Assignee: nobody → doug.turner
Attachment #332208 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #332391 - Flags: review?(chris.double)
Attachment #332208 - Flags: review?(roc)
oh, and is there any asm for ARM that we should be using?
Comment on attachment 332391 [details] [diff] [review]
patch v.2

-DUSE_ASM probably breaks windows.

Also libtheora has moved into media/
Attachment #332391 - Flags: review?(chris.double) → review-
Attached patch patch v.3 (obsolete) — — Splinter Review 

        Attachment #332391 -
        Attachment is obsolete: true

        Attachment #332663 -
        Flags: review?
Assignee: doug.turner → nobody
Status: ASSIGNED → NEW
Component: General → Video/Audio
QA Contact: general → video.audio
Version: unspecified → Trunk
Assignee: nobody → doug.turner

    
  

        Attachment #332663 -
        Flags: review? → review?(ted.mielczarek)

    
    

    
  
Boy, it would be a whole lot nicer to do this as ifdefs directly in the C:

#if defined(__i386__) && defined(__GNUC__)
#define USE_ASM
#endif


    
  

        Attachment #332663 -
        Flags: review?(ted.mielczarek) → review-

    
    

    
  
wow, you can actually read that only once and understand what is going on!

    
    

    
  
chris, how do we get a change like this upstream?

    
    

    
  
I am not sure what OC_X86ASM is suppose to do.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v.4
        — 
          — Splinter Review
      

    


  
code change.  internal.h looks like it is the right place for this.

        Attachment #332663 -
        Attachment is obsolete: true

    
    

    
  
theora-exp uses it.

    
    

    
  
The USE_ASM define in libtheora is actually created by their configure system. We can't add it to any of their .h files, sorry. 

This is why I had it in Makefile.in.

I liase with the Theora developers in #theora on freenode to get things moved upstream if needed.

    
    

    
  
Since I manually create config.h though, via the update.sh script, we could put it in there (if you modify update.sh to append it).

    
    

    
  
we could use patch v.3 if we live with its ugliness.

    
    

    
  
hey, can we get this in soon?

    
    

    
  
yup.  going back and forth in email about this some.  chris said go with v3 in email.  bsmedberg, not as nice as your suggestion, but can't do it -- maybe we can file a bug on the upstream project.

changeset:   16592:7703fa13d46c
tag:         tip
user:        Doug Turner <dougt@meer.net>
date:        Tue Aug 12 07:09:41 2008 -0700
summary:     libtheora's cpu.c assumes x86.  b=449028. r=chris.double@double.co.nz

Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: