diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..813bb29 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,224 @@ +# Contributing + +Thanks for considering a contribution. This document describes how this project actually +works — the conventions below are enforced by git hooks and CI, not by good will. + +## The one rule that shapes everything + +**Zero runtime dependencies.** The `require` section of `composer.json` contains exactly +one entry: `"php": ">=8.2"`. + +This is not minimalism for its own sake. The library exists because decoding a RAW needs +`libraw` or Imagick, which are unavailable on shared hosting without root — and that is the +target. Extracting an already-embedded JPEG needs nothing but byte reading. + +If a change seems to require a runtime dependency, it is out of scope. Symfony, PHPUnit and +GD stay in `require-dev`. + +Two corollaries: + +- **GD is never used to extract.** It builds JPEGs in tests and verifies output with + `imagecreatefromstring()` — never in `src/`. +- **Nothing under `src/` imports `Symfony\`** except `src/Bridge/Symfony/`, which is DI + wiring only. Verify with: + + ```bash + grep -rl 'Symfony\\' src/ --include='*.php' | grep -v '^src/Bridge/' # must be empty + ``` + +## Setup + +```bash +git clone https://github.com/ronan-develop/raw-preview-extractor.git +cd raw-preview-extractor +composer install + +vendor/bin/phpunit # full suite +vendor/bin/phpunit --testsuite unit # fast: forged bytes, no files +vendor/bin/phpunit --testsuite integration # public API end to end +``` + +The suite runs on a clean machine **with no RAW file whatsoever**. If a test needs one, the +test is wrong. + +Optional but recommended — the commit hooks: + +```bash +git config core.hooksPath .githooks +``` + +They reject commits whose message breaks convention, and any staged RAW or binary over 1 MB. + +## Test-driven, strictly + +**No line of `src/` is written before a failing test** — including boilerplate. + +1. Write the test. Run it. **See it fail** — an assumed red is not a red. +2. Write the minimum to pass. +3. Refactor green. + +Test and implementation go in the same commit. A commit never leaves the suite red. + +### Tests use forged bytes, not fixtures + +No camera file is committed to this repository. A RAW carries EXIF (serial number, +sometimes GPS), and a committed binary stays in git history forever. + +Tests build byte sequences in memory — a TIFF header is 8 bytes, an ISO-BMFF box is 8 bytes: + +```php +$header = "II" . pack('v', 42) . pack('V', 8); // little-endian TIFF, IFD at offset 8 +``` + +This tests *better* than real files: it exercises the looping IFD, the out-of-bounds offset, +the `size == 0` box — cases no real camera produces. + +### But forged bytes prove less than you think + +**Reproduce the real structure, not a simplified one.** Every bug found in this project so +far was invisible to a green test suite: + +| Bug | Why tests missed it | +|-------------------------------------|-------------------------------------------------------------------------| +| `findUuid()` searched only the root | Tests put the UUID box at the root. Real CR3s nest it under `moov`. | +| Facade failed on a real CR2 | Every facade test used mocks; none assembled the real parts. | +| CR2 returned 28 MB of sensor data | Canon marks lossless JPEG as `Compression = 6`; the test files did not. | +| G12 raised on a valid file | No test had a candidate that *lied* about its content. | + +A test written from a misunderstanding reproduces the misunderstanding. Before calling a +change done, exercise the public API on a real file (see below). + +## Coverage: ≥ 95 %, enforced + +CI fails below the threshold and lists the uncovered lines. Before adding a test to satisfy +it, **ask whether the line deserves to exist**: + +- **Unreachable** → delete it. Most uncovered lines in this project were dead guards: + `fopen()` checked after `is_file()`, `strlen()` after an already-bounded `fread()`, + `unpack()` guarded when length was guaranteed. A guard that cannot fire is not + defence-in-depth — it is noise that fakes robustness. +- **Reachable, guards a real scenario** (hostile file: looping IFD, absurd `count`, + out-of-bounds offset) → write the test. +- **Hard to reach because of the design** → fix the design. The threshold once revealed + that `IfdEntry` needed a `TiffReader` to read its own values — a cursor disguised as a + value object. + +Coverage went from 83 % to 99 % here mostly by *deleting* code. Never add an artificial test +to move the number. + +## Binary parsing rules + +The file is untrusted input. These are not suggestions: + +- **Never `unpack()`** without checking the byte length first — `fread` near EOF silently + returns less than asked. +- Every offset read from the file is validated against `filesize()` before use. +- **Bound by `filesize()`, never by an arbitrary constant.** A 64 KB cap rejected a valid + Canon 5D whose `MakerNote` is 75 KB. A value cannot exceed the file containing it — that + is the only bound that means anything. +- Endianness is carried explicitly: `v`/`V` (little) or `n`/`N` (big). **Never `S`/`L`** — + they follow the CPU and make parsing machine-dependent. +- Loop guards on IFD chains and box trees: a corrupt or hostile file can point a structure + at itself. +- Validate the `FFD8` magic before returning any JPEG. + +### Corrupt is not the same as missing + +| Exception | When | +| --- | --- | +| `CorruptedFileException` | structurally invalid: out-of-bounds offset, truncated IFD, value larger than the file | +| `PreviewNotFoundException` | valid file, no usable preview — including a tag that lies about its content | +| `UnsupportedFormatException` | not a supported RAW | + +A truncated file is *corrupt*. A tag claiming `Compression = 6` over raw sensor data is +*common in RAWs* — that candidate is skipped, not fatal. Getting this backwards makes the +library reject files it should read. + +All exceptions implement `RawPreviewExtractorException`, so callers degrade with one +`catch`. That contract is tested. + +## Adding a format + +The design intends this to require **no change to existing code**: + +1. Add a case to the `Format` enum. +2. Add a `PreviewParserInterface` implementation. +3. Wire it in `RawPreviewExtractor::createDefault()` and in the bundle's `services.php`. + +The facade resolves through an injected `Format → parser` map — no `switch`, no `match` on +extensions. **If adding a format forces you to touch `RawPreviewExtractor`, the design has +failed** — say so rather than working around it. + +There is no per-model code anywhere, and there should never be. Every camera quirk found so +far was fixed by making a rule *more general*, never by adding a special case. + +## Validating against real files + +```bash +mkdir -p tests/Fixtures/local # git-ignored +cp ~/photos/IMG_0042.CR2 tests/Fixtures/local/ + +php bin/extract-local.php # writes previews to tests/Fixtures/local/output/ +``` + +**Open the output.** A JPEG that decodes is not necessarily the right JPEG — this library +once returned a valid 160×120 thumbnail where a 1620×1080 preview was expected, with every +test green. + +Note that `getimagesizefromstring()` is not enough: it reads the header only and happily +accepts a lossless JPEG that no decoder can open. Use `imagecreatefromstring()`. + +To audit against the public catalogue (400+ models, CC0 files): + +```bash +php bin/audit-cameras.php Canon 25 +php bin/audit-cameras.php all 500 +``` + +Each file is downloaded, tested, then deleted. Failures are the useful output — each one +points at a structure the parser does not yet handle. + +## Commits + +```text + (): +``` + +| Emoji | Type | Emoji | Type | +| --- | --- | --- | --- | +| ✨ | feat | ✅ | test | +| 🔧 | fix | 🏗️ | build | +| 📖 | docs | 🏭 | ci | +| ♻️ | refactor | 🛠️ | chore | +| ⚡ | perf | 🔒 | security | + +Rules the `commit-msg` hook enforces: + +- emoji always present; +- **explicit scope**: a class or module name (`TiffReader`, `Cr3PreviewParser`, `composer`), + never `feat(file)` or `fix(api)`; +- atomic commits — one responsibility each. + +Explain **why** in the body, not what. The diff shows what. + +## Pull requests + +Branch from an issue: `/#-`, e.g. `fix/#27-candidate-without-magic`. +Include `Closes #` in the body. + +All six CI checks must pass — PHP 8.2/Symfony 6.4 at `--prefer-lowest` through PHP +8.4/Symfony 8, plus the coverage threshold. + +## Semver + +The public surface is `RawPreviewExtractorInterface`, `ExtractedPreview`, the `Format` enum +cases, the exception hierarchy, and the bundle's service alias. + +`ExtractedPreview` is `final readonly` with promoted public properties — **everything about +it is public surface**, with no slack. Adding an enum case is minor; renaming a property or +changing which exception fires in a given case is major. + +## License + +MIT. `exiftool` and `libraw` were used as behavioural references only — their output was +compared, never their source read or translated. diff --git a/README.md b/README.md index e48fc09..09795d1 100644 --- a/README.md +++ b/README.md @@ -143,27 +143,31 @@ format parsers break. ### Wider audit -Beyond the table above, the library is audited against the whole -[raw.pixls.us](https://raw.pixls.us/) catalogue — 400+ camera models: +Beyond the table above, the library is audited against the +[raw.pixls.us](https://raw.pixls.us/) catalogue — 400+ camera models, CC0 files: ```bash php bin/audit-cameras.php Canon 25 # 25 Canon models, spread across the range php bin/audit-cameras.php all 500 # everything ``` -It downloads each file, extracts, verifies with `imagecreatefromstring()`, then deletes it. -Latest Canon run — **every EOS body passed**: +Each file is downloaded, extracted, verified with `imagecreatefromstring()`, then deleted. +Latest run — **72 models across the four brands in scope**: -```text -EOS-1D X Mark II EOS 5D Mark IV EOS 30D EOS 100D EOS 500D EOS 1000D -EOS R5 EOS R100 EOS RP EOS M6 EOS Kiss M EOS Rebel T5 -PowerShot G12 PowerShot G1 X Mark II PowerShot SX1 IS PowerShot V1 -``` +| Brand | Passed | Notes | +|-------|--------|----------------------------------| +| Sony | 23/23 | 100 % — A290 (2010) through FX30 | +| Nikon | 20/22 | 90.9 % | +| Apple | 5/6 | 83.3 % | +| Canon | 16/21 | 76.2 % — every EOS body passed | -The remaining failures are compacts whose files genuinely **contain no JPEG preview** — -CHDK-generated DNGs holding an uncompressed 128×96 thumbnail, or legacy `.CRW` files -(pre-CR2, out of scope). `PreviewNotFoundException` is the correct answer there, and the -audit counts it as an expected outcome rather than a defect. +**Every failure was verified by hand, and every one is correct**: those files contain no +JPEG preview at all (zero `FFD8` bytes in the entire file). A Nikon D1H from 2001 carries an +uncompressed 160×120 RGB thumbnail; CHDK-generated compact DNGs do the same; legacy `.CRW` +files predate CR2 and are out of scope. `PreviewNotFoundException` is the right answer, so +the real success rate on files that *have* a preview is 100 %. + +The full list is at the [bottom of this page](#appendix-models-verified-against-real-files). Every preview above is checked with `imagecreatefromstring()` — not just decoded headers, but actually opened. Extraction takes **1–9 ms** on files up to 62 MB: the file is never @@ -175,6 +179,9 @@ check in one command: see [Trying it on your own RAW files](#trying-it-on-your-o ## Contributing +See [CONTRIBUTING.md](CONTRIBUTING.md) for conventions, the TDD workflow, and the binary +parsing rules. In short: + ```bash composer install vendor/bin/phpunit # full suite @@ -220,3 +227,57 @@ MIT — see [LICENSE](LICENSE). `exiftool` and `libraw` were used as behavioural references only. No code was copied from either project. + +--- + +## Appendix: models verified against real files + +Every model below had a real file downloaded, its preview extracted, and the result opened +with `imagecreatefromstring()`. Sources: [raw.pixls.us](https://raw.pixls.us/) (CC0). + +**Canon** — CR2 & CR3 + +```text +EOS 5D EOS 5D Mark II EOS 5D Mark II sRAW1 EOS 5D Mark IV +EOS-1D X Mark II EOS 30D EOS 100D EOS 500D +EOS 1000D EOS Rebel T5 EOS M6 EOS Kiss M +EOS R EOS RP EOS R5 EOS R100 +PowerShot G12 PowerShot G1 X Mark II PowerShot SX1 IS +PowerShot V1 +``` + +**Nikon** — NEF + +```text +D60 D90 D300 D610 D800 D850 D2Xs D3300 +D4 D5100 D5600 D6 D7500 E8400 Z 7 Z 30 +1 AW1 1 J3 1 S2 Coolpix A +``` + +**Sony** — ARW + +```text +DSLR-A290 DSLR-A380 DSLR-A550 DSLR-A850 SLT-A35 SLT-A58 +NEX-5N NEX-7 ILCE-5000 ILCE-6100 ILCE-6600 ILCE-7S +ILCE-7M4 ILCE-7RM3A ILCE-7CM2 ILCA-99M2 ILME-FX30 UMC-R10C +DSC-RX0 DSC-RX100M4 DSC-RX100M7 DSC-RX10M4 DSC-RX1RM2 +``` + +**Apple** — DNG (including ProRAW) + +```text +iPhone 6s Plus iPhone 7 Plus iPhone SE iPhone XS iPhone 12 Pro +``` + +### Known to have no preview + +These files contain **no JPEG at all** — verified byte by byte. `PreviewNotFoundException` +is the correct response, not a defect: + +| Model | What the file actually holds | +|-------------------------------------------------|-------------------------------------------| +| Nikon D1H (2001) | uncompressed 160×120 RGB thumbnail | +| Nikon COOLSCAN V ED | film scanner, no camera preview | +| Apple iPhone 8 | 64 sensor tiles, no JPEG | +| Canon PowerShot SX100 IS, SX510 HS, ELPH 130 IS | CHDK DNGs, uncompressed 128×96 thumbnail | +| Canon PowerShot G7 X, IXY 220F | `.CRW` — pre-CR2 format, out of scope | diff --git a/bin/audit-cameras.php b/bin/audit-cameras.php index bf7ea12..90c900f 100644 --- a/bin/audit-cameras.php +++ b/bin/audit-cameras.php @@ -50,7 +50,11 @@ }; $extractor = RawPreviewExtractor::createDefault(); -$tmp = sys_get_temp_dir() . '/rpe-audit'; + +// Un dossier par processus : plusieurs audits peuvent tourner en parallèle +// (une marque chacun), et le premier qui finissait supprimait le dossier des +// autres — qui écrivaient alors dans le vide et comptaient des faux échecs. +$tmp = sys_get_temp_dir() . '/rpe-audit-' . getmypid(); @mkdir($tmp, 0o755, true); $results = []; @@ -94,6 +98,12 @@ report($results, $stats); +// rmdir échoue sur un dossier non vide : un téléchargement interrompu peut +// laisser un fichier derrière lui. +foreach (glob($tmp . '/*') ?: [] as $leftover) { + @unlink($leftover); +} + @rmdir($tmp); exit($stats['failed'] > 0 ? 1 : 0);