[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_fi
From: |
Richard Wilbur |
Subject: |
Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74 |
Date: |
Sun, 15 Jun 2014 17:18:44 -0600 |
I'm highly in favour of avoiding malloc churn! However, I've got a
beef with Buffers::copy() below:
On Sun, Jun 15, 2014 at 1:56 PM, Bastiaan Jacques <address@hidden> wrote:
> diff --git a/libsound/LiveSound.h b/libsound/LiveSound.h
> index 82df6a5..8551297 100644
> --- a/libsound/LiveSound.h
> +++ b/libsound/LiveSound.h
[...]
> + /// Copy up to the given number of bytes to the given buffer.
> + //
> + /// @to points to a buffer to be written to.
> + /// @bytes number of bytes to be written.
> + /// @return number of bytes actually written.
> + size_t copy(std::uint8_t* to, size_t bytes) {
> + assert(_consumed >= _in_point);
> +
> + size_t bytes_remaining = bytes;
> +
> + for (; _index < _buffers.size(); ++_index) {
> + const SimpleBuffer& buffer = _buffers[_index];
> +
> + size_t to_copy = std::min(bytes_remaining, buffer.size() - _pos);
> +
> + std::copy(buffer.data() + _pos, buffer.data() + _pos + to_copy,
> to);
> + to += to_copy;
> + bytes_remaining -= to_copy;
> + _pos += to_copy;
> +
> + if (_pos == buffer.size()) {
> + ++_index;
> + _pos = 0;
> + break;
> + }
> +
> + if (bytes_remaining == 0) {
> + break;
> + }
> + }
> +
> + size_t written = bytes - bytes_remaining;
> + _consumed += written;
> + return written;
> + }
> +
I would recommend changing the for() loop to a while(_index <
_buffers.size()) and removing the break statement from if (_pos ==
buffer.size()). When _pos == buffer.size(), it means you just
finished copying the present buffer and need to update _index to point
to the next buffer and _pos to start at its beginning, but it doesn't
necessarily mean you are done copying unless bytes_remaining == 0,
right? But if we have the for() loop also incrementing _index, it
seems to me we'll consistently skip buffers.
Suggested wording below:
+ while (_index < _buffers.size()) {
+ const SimpleBuffer& buffer = _buffers[_index];
+
+ size_t to_copy = std::min(bytes_remaining, buffer.size() - _pos);
+
+ std::copy(buffer.data() + _pos, buffer.data() + _pos +
to_copy, to);
+ to += to_copy;
+ bytes_remaining -= to_copy;
+ _pos += to_copy;
+
+ if (_pos == buffer.size()) {
+ ++_index;
+ _pos = 0;
+ }
+
+ if (bytes_remaining == 0) {
+ break;
+ }
+ }
- [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Bastiaan Jacques, 2014/06/15
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74,
Richard Wilbur <=
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Bastiaan Jacques, 2014/06/15
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Richard Wilbur, 2014/06/18
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Gabriele Giacone, 2014/06/18
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Richard Wilbur, 2014/06/23
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Bastiaan Jacques, 2014/06/24
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Richard Wilbur, 2014/06/24
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Bastiaan Jacques, 2014/06/25
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Richard Wilbur, 2014/06/25
- Re: [Gnash-commit] [SCM] Gnash branch, master, updated. release_0_8_9_final-2138-ga9f7f74, Sandro Santilli, 2014/06/26