fixup
This commit is contained in:
55
CODE_REVIEW_FRE-326.md
Normal file
55
CODE_REVIEW_FRE-326.md
Normal file
@@ -0,0 +1,55 @@
|
||||
# Code Review: FRE-326 - Assembly & Rendering
|
||||
|
||||
## Verdict: APPROVED with suggestions
|
||||
|
||||
Reviewed all 6 files in `src/assembly/`:
|
||||
- `__init__.py` (27 lines)
|
||||
- `audio_normalizer.py` (263 lines)
|
||||
- `chapter_builder.py` (328 lines)
|
||||
- `final_renderer.py` (322 lines)
|
||||
- `segment_assembler.py` (233 lines)
|
||||
- `padding_engine.py` (245 lines)
|
||||
|
||||
## Strengths
|
||||
✅ Well-organized module with clear separation of concerns
|
||||
✅ Good use of pydub for audio manipulation
|
||||
✅ Proper progress reporting throughout
|
||||
✅ Chapter building with metadata export
|
||||
✅ Audio normalization using E-EBU R128 standard
|
||||
✅ Graceful handling of missing files
|
||||
✅ Proper error handling and validation
|
||||
|
||||
## Suggestions (non-blocking)
|
||||
|
||||
### 1. final_renderer.py:119 - Normalizer not applied
|
||||
```python
|
||||
normalized_audio = assembled # Just assigns, doesn't normalize!
|
||||
```
|
||||
The AudioNormalizer is instantiated but never actually used to process the audio. The variable should be:
|
||||
```python
|
||||
normalized_audio = self.normalizer.normalize(assembled)
|
||||
```
|
||||
|
||||
### 2. padding_engine.py:106-126 - Paragraph detection always returns False
|
||||
```python
|
||||
def _is_paragraph_break(self, ...) -> bool:
|
||||
...
|
||||
return False # Always returns False!
|
||||
```
|
||||
This makes paragraph padding never applied. Either implement proper detection or remove the feature.
|
||||
|
||||
### 3. audio_normalizer.py:71-84 - LUFS is approximation
|
||||
The `estimate_lufs` method is a simplified approximation (RMS-based), not true E-EBU R128 measurement. Consider using pyloudnorm library for production accuracy.
|
||||
|
||||
### 4. chapter_builder.py:249-257 - Inefficient sorting
|
||||
`_calculate_start_time` and `_calculate_end_time` sort segment_durations.keys() on every call. Consider pre-sorting once.
|
||||
|
||||
### 5. segment_assembler.py:134-136 - Sample rate check
|
||||
```python
|
||||
if audio.frame_rate != target_rate:
|
||||
return audio.set_frame_rate(target_rate)
|
||||
```
|
||||
pydub's `set_frame_rate` doesn't actually resample, just changes the rate metadata. Use `audio.set_frame_rate()` with `audio.set_channels()` for proper conversion.
|
||||
|
||||
## Overall Assessment
|
||||
Solid audio assembly implementation. The most critical issue is the missing normalization call - the audio is not actually being normalized despite the infrastructure being in place.
|
||||
Reference in New Issue
Block a user