573590 - ConvertYCbCrToRGB32 crash [@FastConvertYUVToRGB32Row]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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

Comment 1

[Matthew Gregan [:kinetik]]

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.

Comment 2

[Matthew Gregan [:kinetik]]

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.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Comment 4

[cajbir (:cajbir)]

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?

Comment 5

[Matthew Gregan [:kinetik]]

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 6

[Matthew Gregan [:kinetik]]

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?

Comment 7

[Timothy B. Terriberry (:derf)]

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.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: patch v1

With Tim's changes.

Comment 9

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/cff242513dcf

Comment 11

[Carlos Roberto]

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

Comment 12

[timeless]

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

Comment 13

[Matthew Gregan [:kinetik]]

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.

Comment 14

[Carlos Roberto]

It was already crashing in the first WebM version, before your patch.