Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APU] Fix crash from null sample channel #1876

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philpax
Copy link

@philpax philpax commented Aug 9, 2021

Certain games, such as Forza Motorsport 3, submit XMA data with the stereo flag set with a null second channel. This falls back to mono conversion when the second channel is null, preventing a crash.

Certain games, such as Forza Motorsport 3, submit XMA data with the
stereo flag set with a null second channel. This falls back to mono
conversion when the second channel is null, preventing a crash.
@Triang3l
Copy link
Member

Triang3l commented Aug 9, 2021

This seems like only bypassing a symptom, and may have dangerous effects on other code, not only that depends on this part or generates data for it, but overall XMA decoding wrongly assuming that the track has 2 channels (or not providing the input pointer correctly). If this is safe enough, at least an assertion should be made in debug builds that if the track is stereo, there will be valid data for both channels (otherwise we will just forget about this issue and think that it's a well-defined part of our decoder's contract that the stereo flag may be a lie), but this doesn't seem safe overall at a glance. I think more investigation should be done to find out why this situation happens at all. Can you please provide step-by-step info about how to reproduce this crash in Forza Motorsport 3 and other games (it's nice if there are multiple test cases as the actual reason may vary between games)?

Copy link
Member

@Triang3l Triang3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as changes (more debugging to find the source) requested.

@philpax
Copy link
Author

philpax commented Aug 9, 2021

Great concerns! You're absolutely right about silently ignoring the channel here potentially causing issues:

      ConvertFrame((const uint8_t**)av_frame_->data, bool(data->is_stereo),
                   raw_frame_.data());
      // decoded_consumed_samples_ += kSamplesPerFrame;

      auto byte_count = kBytesPerFrameChannel << data->is_stereo;
      assert_true(output_remaining_bytes >= byte_count);
      output_rb.Write(raw_frame_.data(), byte_count);
      output_remaining_bytes -= byte_count;

This means that it'll always be writing whatever was previously in the second half of raw_frame_. If we wanted to do this properly, perhaps it's worth copying the first channel to the second channel whenever this case happens?

As for a repro, I've only really been testing with FM3, but it seems to happen somewhat inconsistently for me during races and at the post-race screen. I would say probably when there are specific sound effects happening during gameplay; at one point I was getting it consistently after a race.

In terms of stability: this issue appears to have been responsible for all the crashes I've been experiencing in FM3. I was able to successfully complete several races without crashing with this fix applied, so addressing the issue (even if not with this fix) should improve crash numbers.

@gibbed
Copy link
Member

gibbed commented Aug 9, 2021

This problem occurs in Hydro Thunder Hurricane in regular gameplay.

I don't think it's a matter of the input having a single channel and being flagged as stereo, but FFmpeg only giving a single channel back from the decoded data.

@philpax
Copy link
Author

philpax commented Aug 10, 2021

That seems reasonable, but I don't have enough domain knowledge to understand what would cause that - the obvious speculation is that the XMA decoder isn't producing valid data for FFmpeg to decode for the second channel, but I wouldn't know how to investigate that.

What would be the best way to approach this? It appears that this workaround makes several games playable where they weren't before, but it would also be masking a potential decoding error.

@JoelLinn
Copy link
Member

it would be helpful if somebody can dump (a portion of) a stream.
I.e. beginning with the buffer pointer and maybe even up to the end (if the chunk is small enough so it wouldnt violate copyright laws)

@Margen67 Margen67 added the apu label Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants