Skip to content

Conversation

@tangmingcheng
Copy link
Contributor

Summary

This PR refactors the QAVDemuxer::read API to provide better error reporting and resource management while keeping backward compatibility.
Previously, read() returned a QAVPacket, which masked error codes and forced callers to check only whether the packet was valid.
With this change, the new API returns an int (FFmpeg error code), and the QAVPacket is passed by reference.

Key Changes

  • New API

    • Added int read(QAVPacket &outPacket) that returns FFmpeg error codes directly (0 or positive for success, negative values for EOF or errors).
    • Preserves detailed error reporting (e.g., AVERROR_EOF, EAGAIN, network errors such as -10054).
  • Deprecated Old API

    • The old QAVPacket read() is still available but marked with
      QT_DEPRECATED_X("Use read(QAVPacket &outPacket)") for backward compatibility.
    • Internally calls the new read(QAVPacket&).
  • Improved Resource Management

    • Added av_packet_unref(pkt.packet()) before reuse to avoid leaks or stale data.
    • On EOF, properly sets d->eof and flushes bitstream filters by sending NULL to av_bsf_send_packet.
  • Error Propagation

    • Callers can now distinguish between:
      • Success: ret >= 0 && packet.stream()
      • End of stream: ret == AVERROR_EOF
      • Temporary retry: ret == AVERROR(EAGAIN)
      • Fatal error: any other negative value → triggers setError() in QAVPlayer.
  • Bitstream Filter Handling

    • Ensures BSF flushing works at EOF, preventing loss of buffered frames.
    • Returns BSF error codes (bsf_ret) directly if they occur.

}

QAVPacket pkt;
av_packet_unref(pkt.packet());
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting which issues you fixed there? means that could be av_packet_ref before the call to read()?

Copy link
Owner

Choose a reason for hiding this comment

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

Added av_packet_unref(pkt.packet()) before reuse to avoid leaks or stale data.

right, but have you seen actual leaks there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added av_packet_unref(pkt.packet()) as a precaution before reusing the packet. While I haven’t observed concrete leaks in practice, this change ensures that old data does not remain attached to the reused packet. It is a defensive measure against stale state and follows FFmpeg’s own packet reuse recommendations.

QMutexLocker locker(&d->mutex);
d->eof = eof;
if (pkt.packet()->stream_index < d->availableStreams.size())
if (ret >= 0 && pkt.packet()->stream_index < d->availableStreams.size())
Copy link
Owner

Choose a reason for hiding this comment

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

there are several tests failed due to that stream must be available on AVERROR_EOF for the packet,
pkt.setStream should be called of EOF too. Since means need to flush codecs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make sure pkt.setStream() is always called for EOF packets as well. That way, even when ret == AVERROR_EOF, the packet has valid stream metadata, and the decoder can correctly handle flushing. Thanks for pointing this out — this aligns with the test failures you mentioned.

if (packet.stream()) {
QAVPacket packet;
int ret = demuxer.read(packet);
if (ret >= 0 && packet.stream()) {
Copy link
Owner

Choose a reason for hiding this comment

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

// Empty packet points to EOF and it needs to flush codecs

there should be a check for AVERROR_EOF too allow to flush codecs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged. I will add an explicit check for AVERROR_EOF in the caller side (e.g. qavplayer.cpp) so that EOF packets still get processed by codecs instead of being discarded prematurely. This ensures proper flushing of codec buffers.

else
pkt = QAVPacket();
} else if (ret < 0) {
pkt = QAVPacket();
Copy link
Owner

Choose a reason for hiding this comment

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

the packet should not be empty for EOF cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Returning a completely empty packet on EOF is not correct. Instead, I will ensure that pkt.setStream(...) is still called in EOF cases, so the packet has the correct stream index even when it represents EOF. This way, codec flushing can happen as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants