Fix OggPacketSequencePlayback::next_ogg_packet() never returning false

[nikitalita]

This fixes a bug introduced in #80452 (see #80452 (review)). The check that was here caused this function to never return false.

I've tested this with the example projects provided in the issues that the PR purported to fix, and it causes no issues.

[nikitalita]

To clarify the issue here:

Previous to #80452, this function acted as originally intended: It would keep serving up the next packet and returning true until it reached the end of the stream, when it would return false. It also did so silently, without reporting errors, indicating that this was intended to be called to read from the stream until it returned false.

That PR changed that behavior such that, when it reaches the end of the stream, it'll keep serving up the last packet and returning true forever; it won't ever return false. Besides being intuitive, this is just semantically incorrect behavior, as there's no point to have a boolean value as a return value if there's no false case (the user can't set the page to an incorrect value either, as set_page_number is bounds checked).

While none of its current usages in Godot use this to detect the EOS, this is a pretty obvious footgun that can result in infinite loops and could easily become a hazard in the future.

Since none of the Godot usages were using this to detect the EOS (they checked the packet for the e_o_s flag), this change shouldn't affect ogg playback at all. Indeed, I have tested all the projects included with the issues that the previous PR purported to fix, and they all playback fine.

[ellenhp]

@bs-mwoerner I saw you responded in the comment linked in the description here. Does this change seem like it could have unintended consequences? The rationale given seems sound I just want to check.

[bs-mwoerner]

I don't see any problems with this change. When we call next_ogg_packet and it returns false, we consider this an abnormal situation and print a warning. That should still be valid, because we intend to stop reading after seeing an e_o_s packet, so if we ever get false, then something's indeed not quite right and the warning would be appreciated. I tried playing a file with and without looping and with this change in place and the false-paths were never taken. 👍

I say go ahead with the change. There's even still a comment in the header that says it returns "false on error or if there is no next packet".

[ellenhp]

Thanks for weighing in!

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.2.2.