Conversation
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
I was confused why these changes worked and the original code did not. I have come to the conclusion that Chunk::explicit_silence contains a bug that this PR is working around, and I would rather see us fix the bug properly.
Line 45 in b0d3b74
blocks contains one element, but we never actually push that element. The crash should go away if we fix that.
Yes, I will debug Chunk::explicit_silence further |
Signed-off-by: Taym Haddadi <[email protected]>
Taym95
force-pushed
the
fix-iir-filter-silence-crash
branch
from
1394d29 to
a08ed18
Compare
Good catch, indeed, before |
Fix IIRFilter crash on silent blocks and add crash test.
Before fix: `let blocks: SmallVec<[Block; 1]> = SmallVec::new();`
followed by `blocks.as_slice().iter().map(.)`, Because blocks was empty,
the iterator never produced anything, so the resulting Chunk had zero
blocks and caused:
```
panic: index out of bounds: the len is 0 but the index is 0 (thread AudioRenderThread, at /home/runner/.cargo/git/checkouts/media-9074def3f0bdf023/b0d3b74/audio/iir_filter_node.rs:173)
```
Now: we still use SmallVec<[Block; 1]>, but we actually push a block
into it before returning: create Block::default(), call
explicit_silence() on it, blocks.push(block), then Chunk { blocks }.
This ensures the chunk contains one explicit silent block, which is what
downstream code expects.
Actual fix is in:
servo/media#477
This PR uses the new media version containing fix and added crash test.
Testing: added crash test.
Fixes: #41085
Signed-off-by: Taym Haddadi <[email protected]>
Fix IIRFilter crash on silent blocks and add crash test.
Before fix: `let blocks: SmallVec<[Block; 1]> = SmallVec::new();`
followed by `blocks.as_slice().iter().map(.)`, Because blocks was empty,
the iterator never produced anything, so the resulting Chunk had zero
blocks and caused:
```
panic: index out of bounds: the len is 0 but the index is 0 (thread AudioRenderThread, at /home/runner/.cargo/git/checkouts/media-9074def3f0bdf023/b0d3b74/audio/iir_filter_node.rs:173)
```
Now: we still use SmallVec<[Block; 1]>, but we actually push a block
into it before returning: create Block::default(), call
explicit_silence() on it, blocks.push(block), then Chunk { blocks }.
This ensures the chunk contains one explicit silent block, which is what
downstream code expects.
Actual fix is in:
servo/media#477
This PR uses the new media version containing fix and added crash test.
Testing: added crash test.
Fixes: #41085
Signed-off-by: Taym Haddadi <[email protected]>
Fix IIRFilter crash on silent blocks and add crash test.
Before fix: `let blocks: SmallVec<[Block; 1]> = SmallVec::new();`
followed by `blocks.as_slice().iter().map(.)`, Because blocks was empty,
the iterator never produced anything, so the resulting Chunk had zero
blocks and caused:
```
panic: index out of bounds: the len is 0 but the index is 0 (thread AudioRenderThread, at /home/runner/.cargo/git/checkouts/media-9074def3f0bdf023/b0d3b74/audio/iir_filter_node.rs:173)
```
Now: we still use SmallVec<[Block; 1]>, but we actually push a block
into it before returning: create Block::default(), call
explicit_silence() on it, blocks.push(block), then Chunk { blocks }.
This ensures the chunk contains one explicit silent block, which is what
downstream code expects.
Actual fix is in:
servo/media#477
This PR uses the new media version containing fix and added crash test.
Testing: added crash test.
Fixes: #41085
Signed-off-by: Taym Haddadi <[email protected]>
2 participants
fixes servo/servo#41085