[FFmpeg-devel] VP8 on win64

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Tue, Aug 24, 2010 at 12:13:06AM -0400, Ronald S. Bultje wrote:
>> Hi,
>> 
>> On Sun, Aug 22, 2010 at 10:34 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
>> > On Sun, Aug 22, 2010 at 11:19 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> >> On Sun, Aug 22, 2010 at 10:17 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
>> >>> On Sun, Aug 22, 2010 at 11:06 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> >>>> 2010/8/22 M?ns Rullg?rd <mans at mansr.com>:
>> >>>>> All the vp8 tests are failing on win64 with an error code of 5 rather
>> >>>>> than the more usual 1 (which is how the other tests fail). ?Does
>> >>>>> anyone a) know what code 5 means, or b) have any idea what's going
>> >>>>> wrong?
>> >>>>
>> >>>> I think attached patch fixes it, at least one of the crashes is now
>> >>>> working for me. Need better understanding of Windows to be able to
>> >>>> figure out how to run "make fate", but I'm hoping the win-experts can
>> >>>> help me with that. ;-).
>> >>>
>> >>> With that patch, only test vectors 003, 004, and 007 fail.
>> >>
>> >> Ah yes I missed the bilin ones.
>> >
>> > 3 and 7 still failed on make fate but not when I tried through gdb.
>> 
>> I've been looking into these. The problem is that on win64, for test 3
>> and 7, it only outputs the first two frames of the video instead of
>> all.
>> 
>> It turns out it drops all other frames in do_video_out() because the
>> PTS is set to zero all the way back before output_packet() is called
>> in ffmpeg.c. This is not the case on my OSX box (also 64bit), which
>> passes make fate. The input PTS from av_read_frame() is the same on
>> both systems, so something happens between the call to av_read_frame()
>> and the call to output_packet() in ffmpeg.c:transcode(). Using
>> watchpoints, I think I've tracked it down to ffmpeg.c:2598:
>> 
>> 2595            if(input_files_ts_scale[file_index][pkt.stream_index]){
>> 2596                if(pkt.pts != AV_NOPTS_VALUE)
>> 2597                    pkt.pts *=
>> input_files_ts_scale[file_index][pkt.stream_index];
>> 2598                if(pkt.dts != AV_NOPTS_VALUE)
>> 2599                    pkt.dts *=
>> input_files_ts_scale[file_index][pkt.stream_index];
>> 2600            }
>> 
>> These modify the correct pts of 2 to a system-dependent 0 on win64
>> only (but not on Linux). I think this happens because
>> input_files_ts_scale[][] is a double[][]. The value, at the time of
>> the comparison, is still 0.0 (zero), with 8 zero bytes in memory (or
>> do doubles have more on win64?), however the if does trigger and the
>> code inside is executed.
>> 
>> I don't know why this doesn't happen for other testfiles,
>
>> but we know
>> that general consensus is that you should never use doubles for exact
>> comparisons.
>
> general consensus amongth whom?

Amongst people using floating-point.  It is true that _computed_
floating-point numbers should never be compared for equality since
rounding rules mean a few bits may differ even between compliant
implementations.  To be safe, one must calculate the accuracy of the
result and do a range comparison.

However, setting a float to an exact constant (such as zero) and later
comparing with the same constant is of course safe.

>> Therefore, I think the check should be changed into
>> something more intelligent, or maybe the default values should be
>> changed to -1 so we can check for if (val > 0) instead of if (val).
>> The attached patch is a poor attempt at doing so, probably not right
>> for SVN, but it hopefully shows how to fix this bug.
>
> if if(0.0) is true this is a bug period

Definitely.

> try with all asm disabled, if it still happens the compiler is buggy
> if it doesnt happen look at the generated code and think about if it
> could be affected by asm not declaring which sse registers are clobbered

Didn't someone already say it all passes with asm disabled.  I
strongly suspect the missing clobbers here.

-- 
M?ns Rullg?rd
mans at mansr.com