ConvertYCbCrToRGB32 crash [@FastConvertYUVToRGB32Row]

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

32-bit debug builds on Fedora 12 (using GCC 4.4.3) crash in FastConvertYUVToRGB32Row at the pusha instruction the first time it's called.  Optimized builds work fine.

We're not hitting this on the Fedora 12 build machines because they're using a custom-built GCC 4.3.3.  Fedora 12 ships with 4.4.3.

#0  0x025dfd00 in FastConvertYUVToRGB32Row ()
   from /home/kinetik/mozilla-central/obj/dist/bin/libxul.so
#1  0x01ec65f9 in mozilla::gfx::ConvertYCbCrToRGB32 (y_buf=
    0xac006840 "\346\346\346\346\346\346\346", <incomplete sequence \346>..., u_buf=
    0xac04ca30 "\200\200\200\200\200\200\200\200"..., v_buf=
    0xac05f330 "\200\200\200\200\200\200\200\200"..., rgb_buf=
    0xabd01000 "\245\245\245\245\245\245\245\245"..., pic_x=0, pic_y=0, pic_width=640, pic_height=
    360, y_pitch=704, uv_pitch=352, rgb_pitch=2560, yuv_type=YV12)
    at /home/kinetik/mozilla-central/gfx/ycbcr/yuv_convert.cpp:73
#2  0x01eb77ca in mozilla::layers::BasicPlanarYCbCrImage::SetData (this=0xacdacf70, aData=...)
    at /home/kinetik/mozilla-central/gfx/layers/basic/BasicImages.cpp:164
#3  0x01441fc2 in VideoData::Create (aInfo=..., aContainer=0xaf9296e0, aOffset=-1, aTime=0,
    aEndTime=40, aBuffer=..., aKeyframe=1, aTimecode=-1)
    at /home/kinetik/mozilla-central/content/media/nsBuiltinDecoderReader.cpp:163
#4  0x0145c25c in nsWebMReader::DecodeVideoFrame (this=0xacd87400, aKeyframeSkip=@0xacbfeeec,
    aTimeThreshold=0) at /home/kinetik/mozilla-central/content/media/webm/nsWebMReader.cpp:612
(gdb) x/i $pc
0x25dfd00 <FastConvertYUVToRGB32Row>:   pusha
(gdb) info reg
eax            0xac006840       -1409259456
ecx            0x1      1
edx            0x0      0
ebx            0x25a5454        39474260
esp            0xacbfec2c       0xacbfec2c
ebp            0xacbfec88       0xacbfec88
esi            0x0      0
edi            0x0      0
eip            0x25dfd00        0x25dfd00 <FastConvertYUVToRGB32Row>
eflags         0x10202  [ IF RF ]
cs             0x73     115
ss             0x7b     123
ds             0x7b     123
es             0x7b     123
fs             0x0      0
gs             0x33     51
This happens because FastConvertYUVToRGB32Row ends up in the .data section (which is not executable) in my debug build.  In my optimized build, it's placed in the .text section with the rest of the code.
Looks like changing:

  asm(
  ".global FastConvertYUVToRGB32Row\n"
"FastConvertYUVToRGB32Row:\n"

to:

  asm(
  ".text\n"
  ".global FastConvertYUVToRGB32Row\n"
  ".type FastConvertYUVToRGB32Row, @function\n"
"FastConvertYUVToRGB32Row:\n"

...is sufficient to fix this.  I'll post a patch shortly--I'd like to fix bug 572034#c15 at the same time, so I'll include the changes required for that in this patch.
Posted patch patch v0 (obsolete) — Splinter Review
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #452968 - Flags: review?(chris.double)
Comment on attachment 452968 [details] [diff] [review]
patch v0

Best to get someone who knows more about assembly than me to review this, sorry. Maybe Tim?
Attachment #452968 - Flags: review?(chris.double)
It doesn't change the logic.  It contains two additional documented assembler directives, and transforms the labels into a form that is considered a local label in both gas and OS X's as.
Comment on attachment 452968 [details] [diff] [review]
patch v0

Tim, I know this isn't your code, but would you mind taking a look at it?
Attachment #452968 - Flags: review?(tterribe)
Reviewing in a comment since I don't have permission to edit the attachment:

gcc remembers each time it emits a section directive and won't emit redundant ones, so by changing the section in an asm block, you can alter the section of anything that would have followed it in the compilation unit. The only way I know of to avoid this is to use the ELF directive ".section .text" and add a ".previous" directive at the end of the block to revert to the previous section.

These directives are only supported for ELF targets. Since the name mangling already limits the targets you can support, this is probably not a big deal, but I would suggest adding a comment to document this, as it may not be immediately obvious to someone not familiar with asm directives.

r+ with changes

If you want to do this in a more generic way in the future, it would be better to use a separate .S file.
Posted patch patch v1Splinter Review
With Tim's changes.
Attachment #452968 - Attachment is obsolete: true
Attachment #453224 - Flags: review+
Attachment #452968 - Flags: review?(tterribe)
http://hg.mozilla.org/mozilla-central/rev/cff242513dcf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 577645
I am getting this bug yet.

Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100709 Minefield/4.0b2pre

bp-481adb9b-012e-42de-a12a-396bd2100709
Depends on: 577645
sorry, my fault for not looking more carefully, they fixed a crash in this function by introducing code into the same function which doesn't work with your processor
My patch didn't introduce any new code, movntq was already present.  The fact that the patch includes a patch makes it harder to see what was really changed (if you're looking at the included patch rather than the changes directly) due to the extra source context included.
No longer depends on: 577645
It was already crashing in the first WebM version, before your patch.
You need to log in before you can comment on or make changes to this bug.