Merge pull request #132 from jpirnay/chore-indexing

refactor: Speed up indexing (less visual updates + elimination of SD tmp file)
2026-04-25 11:25:36 +02:00
parent 88855d0258 c8a2332ed3
commit 8611bbf637
5 changed files with 181 additions and 133 deletions
@@ -189,8 +189,8 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
                                const uint8_t paragraphAlignment, const uint16_t viewportWidth,
                                const uint16_t viewportHeight, const bool hyphenationEnabled, const bool embeddedStyle,
                                const uint8_t imageRendering, const std::function<void(int)>& progressFn) {
+  const uint32_t phaseTotalStart = millis();
  const auto localPath = epub->getSpineItem(spineIndex).href;
-  const auto tmpHtmlPath = epub->getCachePath() + "/.tmp_" + std::to_string(spineIndex) + ".html";

  // Create cache directory if it doesn't exist
  {
@@ -198,42 +198,14 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
    Storage.mkdir(sectionsDir.c_str());
  }

-  // Retry logic for SD card timing issues
-  bool success = false;
-  uint32_t fileSize = 0;
-  for (int attempt = 0; attempt < 3 && !success; attempt++) {
-    if (attempt > 0) {
-      LOG_DBG("SCT", "Retrying stream (attempt %d)...", attempt + 1);
-      delay(50);  // Brief delay before retry
-    }
-
-    // Remove any incomplete file from previous attempt before retrying
-    if (Storage.exists(tmpHtmlPath.c_str())) {
-      Storage.remove(tmpHtmlPath.c_str());
-    }
-
-    FsFile tmpHtml;
-    if (!Storage.openFileForWrite("SCT", tmpHtmlPath, tmpHtml)) {
-      continue;
-    }
-    success = epub->readItemContentsToStream(localPath, tmpHtml, 1024);
-    fileSize = tmpHtml.size();
-    tmpHtml.close();
-
-    // If streaming failed, remove the incomplete file immediately
-    if (!success && Storage.exists(tmpHtmlPath.c_str())) {
-      Storage.remove(tmpHtmlPath.c_str());
-      LOG_DBG("SCT", "Removed incomplete temp file after failed attempt");
-    }
-  }
-
-  if (!success) {
-    LOG_ERR("SCT", "Failed to stream item contents to temp file after retries");
+  // Get inflated size up-front so the parser can choose progress granularity.
+  const uint32_t phaseSetupStart = millis();
+  size_t inflatedSize = 0;
+  if (!epub->getItemSize(localPath, &inflatedSize)) {
+    LOG_ERR("SCT", "Failed to get inflated size for %s", localPath.c_str());
    return false;
  }

-  LOG_DBG("SCT", "Streamed temp HTML to %s (%d bytes)", tmpHtmlPath.c_str(), fileSize);
-
  if (!Storage.openFileForWrite("SCT", filePath, file)) {
    return false;
  }
@@ -270,16 +242,14 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
  }

  ChapterHtmlSlimParser visitor(
-      epub, tmpHtmlPath, renderer, fontId, lineCompression, extraParagraphSpacing, paragraphAlignment, viewportWidth,
-      viewportHeight, hyphenationEnabled,
+      epub, renderer, fontId, lineCompression, extraParagraphSpacing, paragraphAlignment, viewportWidth, viewportHeight,
+      hyphenationEnabled,
      [this, &lut](std::unique_ptr<Page> page) { lut.emplace_back(this->onPageComplete(std::move(page))); },
      embeddedStyle, contentBase, imageBasePath, imageRendering, std::move(tocAnchors), progressFn, cssParser);
  Hyphenator::setPreferredLanguage(epub->getLanguage());
-  success = visitor.parseAndBuildPages();

-  Storage.remove(tmpHtmlPath.c_str());
-  if (!success) {
-    LOG_ERR("SCT", "Failed to parse XML and build pages");
+  if (!visitor.setup(inflatedSize)) {
+    LOG_ERR("SCT", "Failed to set up chapter parser");
    file.close();
    Storage.remove(filePath.c_str());
    if (cssParser) {
@@ -287,6 +257,29 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
    }
    return false;
  }
+  const uint32_t setupMs = millis() - phaseSetupStart;
+
+  // Stream EPUB item content directly into the parser — no temp file, no second SD pass.
+  const uint32_t phaseParseStart = millis();
+  const bool streamOk = epub->readItemContentsToStream(localPath, visitor, 1024);
+  const bool finalizeOk = visitor.finalize();
+  bool success = streamOk && finalizeOk && visitor.streamSucceeded();
+  const uint32_t parseMs = millis() - phaseParseStart;
+  // streamMs is no longer a separate phase (SD-write of temp file is gone); keep the
+  // log breakdown stable by reporting it as 0.
+  constexpr uint32_t streamMs = 0;
+
+  const uint32_t phaseFinalizeStart = millis();
+  if (!success) {
+    LOG_ERR("SCT", "Failed to parse XML and build pages (stream=%d finalize=%d)", streamOk ? 1 : 0, finalizeOk ? 1 : 0);
+    file.close();
+    Storage.remove(filePath.c_str());
+    if (cssParser) {
+      cssParser->clear();
+    }
+    return false;
+  }
+  const uint32_t fileSize = static_cast<uint32_t>(inflatedSize);

  const uint32_t lutOffset = file.position();
  bool hasFailedLutRecords = false;
@@ -353,6 +346,10 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
    return false;
  }
  this->lut = std::move(lut);
+  const uint32_t finalizeMs = millis() - phaseFinalizeStart;
+  const uint32_t totalMs = millis() - phaseTotalStart;
+  LOG_DBG("SCT", "createSectionFile spine=%d total=%ums (stream=%u setup=%u parse=%u finalize=%u) pages=%u bytes=%u",
+          spineIndex, totalMs, streamMs, setupMs, parseMs, finalizeMs, pageCount, fileSize);
  return true;
 }

@@ -19,8 +19,16 @@
 const char* HEADER_TAGS[] = {"h1", "h2", "h3", "h4", "h5", "h6"};
 constexpr int NUM_HEADER_TAGS = sizeof(HEADER_TAGS) / sizeof(HEADER_TAGS[0]);

-// Minimum file size (in bytes) to show indexing popup - smaller chapters don't benefit from it
-constexpr size_t MIN_SIZE_FOR_POPUP = 10 * 1024;  // 10KB
+// Size thresholds (bytes of XHTML) controlling indexing popup behavior.
+// Each progress callback costs ~640ms of e-ink refresh, so we trade granularity off
+// against indexing time based on expected duration.
+//   < 15KB:  no popup at all - indexing finishes faster than the popup would draw
+//   < 30KB:  popup only (one refresh up-front, no mid-parse updates)
+//   < 80KB:  popup + one heartbeat at 50%
+//   >= 80KB: popup + ticks at 25/50/75%
+constexpr size_t MIN_SIZE_FOR_POPUP = 15 * 1024;
+constexpr size_t SIZE_FOR_PROGRESS_HEARTBEAT = 30 * 1024;
+constexpr size_t SIZE_FOR_PROGRESS_FINE = 80 * 1024;
 constexpr size_t PARSE_BUFFER_SIZE = 1024;

 const char* BLOCK_TAGS[] = {"p", "li", "div", "br", "blockquote", "pre"};
@@ -1313,7 +1321,17 @@ void XMLCALL ChapterHtmlSlimParser::endElement(void* userData, const XML_Char* n
  }
 }

-bool ChapterHtmlSlimParser::parseAndBuildPages() {
+ChapterHtmlSlimParser::~ChapterHtmlSlimParser() {
+  if (activeParser) {
+    XML_StopParser(activeParser, XML_FALSE);
+    XML_SetElementHandler(activeParser, nullptr, nullptr);
+    XML_SetCharacterDataHandler(activeParser, nullptr);
+    XML_ParserFree(activeParser);
+    activeParser = nullptr;
+  }
+}
+
+bool ChapterHtmlSlimParser::setup(const size_t totalInflatedSize) {
  auto paragraphAlignmentBlockStyle = BlockStyle();
  paragraphAlignmentBlockStyle.textAlignDefined = true;
  // Resolve None sentinel to Justify for initial block (no CSS context yet)
@@ -1323,101 +1341,116 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() {
  paragraphAlignmentBlockStyle.alignment = align;
  startNewTextBlock(paragraphAlignmentBlockStyle);

-  const XML_Parser parser = XML_ParserCreate(nullptr);
-  int done;
-
+  XML_Parser parser = XML_ParserCreate(nullptr);
  if (!parser) {
    LOG_ERR("EHP", "Couldn't allocate memory for parser");
    return false;
  }

-  // Handle HTML entities (like &nbsp;) that aren't in XML spec or DTD
-  // Using DefaultHandlerExpand preserves normal entity expansion from DOCTYPE
+  // Handle HTML entities (like &nbsp;) that aren't in XML spec or DTD.
+  // Using DefaultHandlerExpand preserves normal entity expansion from DOCTYPE.
  XML_SetDefaultHandlerExpand(parser, defaultHandlerExpand);
-
-  FsFile file;
-  if (!Storage.openFileForRead("EHP", filepath, file)) {
-    XML_ParserFree(parser);
-    return false;
-  }
-
-  const size_t totalFileSize = file.size();
-  size_t bytesRead = 0;
-  int lastReportedProgress = -1;
-
-  // Show initial progress popup for files above threshold.
-  if (progressFn && totalFileSize >= MIN_SIZE_FOR_POPUP) {
-    progressFn(0);
-  }
-
  XML_SetUserData(parser, this);
  XML_SetElementHandler(parser, startElement, endElement);
  XML_SetCharacterDataHandler(parser, characterData);
  activeParser = parser;

-  // Compute the time taken to parse and build pages
-  const uint32_t chapterStartTime = millis();
-  do {
-    void* const buf = XML_GetBuffer(parser, PARSE_BUFFER_SIZE);
-    if (!buf) {
-      LOG_ERR("EHP", "Couldn't allocate memory for buffer");
-      XML_StopParser(parser, XML_FALSE);                // Stop any pending processing
-      XML_SetElementHandler(parser, nullptr, nullptr);  // Clear callbacks
-      XML_SetCharacterDataHandler(parser, nullptr);
-      activeParser = nullptr;
-      XML_ParserFree(parser);
-      file.close();
-      return false;
+  totalStreamSize = totalInflatedSize;
+  bytesStreamed = 0;
+  lastReportedProgress = -1;
+  streamFailed = false;
+  streamStartTimeMs = millis();
+
+  // Choose progress granularity by chapter size. Each callback drives a full-screen
+  // e-ink refresh (~640ms), so smaller chapters skip mid-parse ticks entirely.
+  // progressStepPercent == 0 means "popup only, no mid-parse updates".
+  progressStepPercent = 0;
+  if (totalStreamSize >= SIZE_FOR_PROGRESS_FINE) {
+    progressStepPercent = 25;
+  } else if (totalStreamSize >= SIZE_FOR_PROGRESS_HEARTBEAT) {
+    progressStepPercent = 50;
  }

-    const size_t len = file.read(buf, PARSE_BUFFER_SIZE);
-    bytesRead += len;
+  // Show initial progress popup for files above threshold.
+  if (progressFn && totalStreamSize >= MIN_SIZE_FOR_POPUP) {
+    progressFn(0);
+  }
+  return true;
+}

-    // Report progress in 5% increments to limit e-ink refreshes.
-    if (progressFn && totalFileSize >= MIN_SIZE_FOR_POPUP) {
-      const int progress = static_cast<int>(bytesRead * 100 / totalFileSize);
-      if (progress / 5 > lastReportedProgress / 5) {
+size_t ChapterHtmlSlimParser::write(const uint8_t data) { return write(&data, 1); }
+
+size_t ChapterHtmlSlimParser::write(const uint8_t* buffer, const size_t size) {
+  if (size == 0) return 0;
+  if (!activeParser || streamFailed) return 0;
+
+  size_t remaining = size;
+  const uint8_t* cursor = buffer;
+  while (remaining > 0) {
+    const size_t chunk = remaining < PARSE_BUFFER_SIZE ? remaining : PARSE_BUFFER_SIZE;
+    void* const buf = XML_GetBuffer(activeParser, static_cast<int>(chunk));
+    if (!buf) {
+      LOG_ERR("EHP", "Couldn't allocate buffer");
+      streamFailed = true;
+      return 0;
+    }
+    memcpy(buf, cursor, chunk);
+
+    bytesStreamed += chunk;
+    // The streaming source doesn't know "this was the last chunk" — pass isFinal=false
+    // here and let finalize() emit the terminating empty parse with isFinal=true.
+    if (XML_ParseBuffer(activeParser, static_cast<int>(chunk), 0) == XML_STATUS_ERROR) {
+      LOG_ERR("EHP", "Parse error at line %lu:\n%s", XML_GetCurrentLineNumber(activeParser),
+              XML_ErrorString(XML_GetErrorCode(activeParser)));
+      streamFailed = true;
+      return 0;
+    }
+
+    cursor += chunk;
+    remaining -= chunk;
+  }
+
+  // Report progress at the granularity chosen up-front (see progressStepPercent).
+  // Skip the 100% callback — the page render that follows immediately replaces the popup,
+  // so the final tick is wasted work.
+  if (progressFn && progressStepPercent > 0 && totalStreamSize > 0) {
+    const int progress = static_cast<int>(bytesStreamed * 100 / totalStreamSize);
+    if (progress < 100 && progress / progressStepPercent > lastReportedProgress / progressStepPercent) {
      lastReportedProgress = progress;
      progressFn(progress);
    }
  }

-    if (len == 0 && file.available() > 0) {
-      LOG_ERR("EHP", "File read error");
-      XML_StopParser(parser, XML_FALSE);                // Stop any pending processing
-      XML_SetElementHandler(parser, nullptr, nullptr);  // Clear callbacks
-      XML_SetCharacterDataHandler(parser, nullptr);
-      activeParser = nullptr;
-      XML_ParserFree(parser);
-      file.close();
+  return size;
+}
+
+bool ChapterHtmlSlimParser::finalize() {
+  if (!activeParser) {
    return false;
  }

-    done = file.available() == 0;
-
-    if (XML_ParseBuffer(parser, static_cast<int>(len), done) == XML_STATUS_ERROR) {
-      LOG_ERR("EHP", "Parse error at line %lu:\n%s", XML_GetCurrentLineNumber(parser),
-              XML_ErrorString(XML_GetErrorCode(parser)));
-      XML_StopParser(parser, XML_FALSE);                // Stop any pending processing
-      XML_SetElementHandler(parser, nullptr, nullptr);  // Clear callbacks
-      XML_SetCharacterDataHandler(parser, nullptr);
-      activeParser = nullptr;
-      XML_ParserFree(parser);
-      file.close();
-      return false;
+  bool success = !streamFailed;
+  if (success) {
+    // Emit terminating empty parse so Expat finalizes any pending tokens.
+    if (XML_ParseBuffer(activeParser, 0, 1) == XML_STATUS_ERROR) {
+      LOG_ERR("EHP", "Parse error at line %lu (finalize):\n%s", XML_GetCurrentLineNumber(activeParser),
+              XML_ErrorString(XML_GetErrorCode(activeParser)));
+      success = false;
+      streamFailed = true;
    }
-  } while (!done);
-  const uint32_t totalTimeMs = millis() - chapterStartTime;
+  }
+
+  XML_StopParser(activeParser, XML_FALSE);
+  XML_SetElementHandler(activeParser, nullptr, nullptr);
+  XML_SetCharacterDataHandler(activeParser, nullptr);
+  XML_ParserFree(activeParser);
+  activeParser = nullptr;
+
+  const uint32_t totalTimeMs = millis() - streamStartTimeMs;
  LOG_DBG("EHP", "Time to parse and build pages: %lu ms", totalTimeMs);

-  XML_StopParser(parser, XML_FALSE);                // Stop any pending processing
-  XML_SetElementHandler(parser, nullptr, nullptr);  // Clear callbacks
-  XML_SetCharacterDataHandler(parser, nullptr);
-  activeParser = nullptr;
-  XML_ParserFree(parser);
-  file.close();
-
-  // Process last page if there is still text
+  // Process last page if there is still text. Done unconditionally so that a partial
+  // success scenario still flushes whatever pages were produced.
  if (currentTextBlock) {
    makePages();
    if (!pendingAnchorId.empty()) {
@@ -1429,7 +1462,7 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() {
    currentTextBlock.reset();
  }

-  return true;
+  return success;
 }

 ParsedText::LineProcessResult ChapterHtmlSlimParser::addLineToPage(std::shared_ptr<TextBlock> line,
@@ -1,5 +1,6 @@
 #pragma once

+#include <Print.h>
 #include <expat.h>

 #include <climits>
@@ -22,9 +23,8 @@ class Epub;

 #define MAX_WORD_SIZE 200

-class ChapterHtmlSlimParser {
+class ChapterHtmlSlimParser final : public Print {
  std::shared_ptr<Epub> epub;
-  const std::string& filepath;
  GfxRenderer& renderer;
  std::function<void(std::unique_ptr<Page>)> completePageFn;
  std::function<void(int)> progressFn;  // Progress callback (0-100)
@@ -105,11 +105,19 @@ class ChapterHtmlSlimParser {
  };
  std::vector<ParagraphLutEntry> paragraphLutPerPage;  // deep LUT: one entry per page

-  // Active parser handle during parseAndBuildPages(), nullptr otherwise.
+  // Active parser handle during streaming, nullptr otherwise.
  // Stored as a member so page-break sites (addLineToPage, image breaks) can call
  // XML_GetCurrentByteIndex without needing the parser threaded through every call.
  XML_Parser activeParser = nullptr;

+  // Streaming state for the Print-derived parsing API.
+  size_t totalStreamSize = 0;
+  size_t bytesStreamed = 0;
+  int lastReportedProgress = -1;
+  int progressStepPercent = 0;
+  bool streamFailed = false;
+  uint32_t streamStartTimeMs = 0;
+
  // Footnote link tracking
  bool insideFootnoteLink = false;
  int footnoteLinkDepth = -1;
@@ -138,8 +146,8 @@ class ChapterHtmlSlimParser {
  static void XMLCALL endElement(void* userData, const XML_Char* name);

 public:
-  explicit ChapterHtmlSlimParser(std::shared_ptr<Epub> epub, const std::string& filepath, GfxRenderer& renderer,
-                                 const int fontId, const float lineCompression, const bool extraParagraphSpacing,
+  explicit ChapterHtmlSlimParser(std::shared_ptr<Epub> epub, GfxRenderer& renderer, const int fontId,
+                                 const float lineCompression, const bool extraParagraphSpacing,
                                 const uint8_t paragraphAlignment, const uint16_t viewportWidth,
                                 const uint16_t viewportHeight, const bool hyphenationEnabled,
                                 const std::function<void(std::unique_ptr<Page>)>& completePageFn,
@@ -150,7 +158,6 @@ class ChapterHtmlSlimParser {
                                 const CssParser* cssParser = nullptr)

      : epub(epub),
-        filepath(filepath),
        renderer(renderer),
        fontId(fontId),
        lineCompression(lineCompression),
@@ -168,8 +175,22 @@ class ChapterHtmlSlimParser {
        imageBasePath(imageBasePath),
        tocAnchors(std::move(tocAnchors)) {}

-  ~ChapterHtmlSlimParser() = default;
-  bool parseAndBuildPages();
+  ~ChapterHtmlSlimParser() override;
+
+  // Streaming parse lifecycle. Caller flow:
+  //   parser.setup(totalInflatedSize);
+  //   epub->readItemContentsToStream(href, parser, ...);
+  //   parser.finalize();
+  // Returns false from setup() on parser allocation failure; check streamSucceeded()
+  // after finalize() to detect a parse error mid-stream.
+  bool setup(size_t totalInflatedSize);
+  bool finalize();
+  [[nodiscard]] bool streamSucceeded() const { return !streamFailed; }
+
+  // Print interface — fed by Epub::readItemContentsToStream.
+  size_t write(uint8_t) override;
+  size_t write(const uint8_t* buffer, size_t size) override;
+
  ParsedText::LineProcessResult addLineToPage(std::shared_ptr<TextBlock> line, bool lineEndsWithHyphenatedWord,
                                              bool suppressHyphenationRetry);
  const std::vector<std::pair<std::string, uint16_t>>& getAnchors() const { return anchorData; }
@@ -1236,7 +1236,10 @@ void EpubReaderActivity::render(RenderLock&& lock) {
      Rect popupRect{};
      const auto progressFn = [this, &popupRect](int progress) {
        if (popupRect.width == 0) {
+          // Drawing the popup already does a full refresh, which serves as the
+          // 0% indication; no need to follow it with a redundant fillPopupProgress.
          popupRect = GUI.drawPopup(renderer, tr(STR_INDEXING));
+          return;
        }
        GUI.fillPopupProgress(renderer, popupRect, progress);
      };
@@ -131,8 +131,7 @@ bool BmpViewerActivity::renderBmpImage(const bool showControls) {
  FsFile file;
  const auto pageWidth = renderer.getScreenWidth();
  const auto pageHeight = renderer.getScreenHeight();
-  Rect popupRect = GUI.drawPopup(renderer, tr(STR_LOADING_POPUP));
-  GUI.fillPopupProgress(renderer, popupRect, 20);
+  GUI.drawPopup(renderer, tr(STR_LOADING_POPUP));

  if (!Storage.openFileForRead("BMP", filePath, file)) {
    return false;
@@ -148,8 +147,6 @@ bool BmpViewerActivity::renderBmpImage(const bool showControls) {
  computeCenteredImagePlacement(bitmap.getWidth(), bitmap.getHeight(), pageWidth, pageHeight, x, y, renderWidth,
                                renderHeight);

-  GUI.fillPopupProgress(renderer, popupRect, 50);
-
  bmpHasGreyscale = bitmap.hasGreyscale();
  // Only render in grayscale when the bitmap actually carries greyscale data AND the user has it enabled.
  const bool renderGrayscale = bmpHasGreyscale && grayscaleDisplay;
@@ -196,8 +193,7 @@ bool BmpViewerActivity::renderDecodedImage(const bool showControls) {
  RenderLock lock(*this);
  const auto pageWidth = renderer.getScreenWidth();
  const auto pageHeight = renderer.getScreenHeight();
-  Rect popupRect = GUI.drawPopup(renderer, tr(STR_LOADING_POPUP));
-  GUI.fillPopupProgress(renderer, popupRect, 20);
+  GUI.drawPopup(renderer, tr(STR_LOADING_POPUP));

  ImageToFramebufferDecoder* decoder = ImageDecoderFactory::getDecoder(filePath);
  if (!decoder) {
@@ -212,8 +208,6 @@ bool BmpViewerActivity::renderDecodedImage(const bool showControls) {
  int x, y, renderWidth, renderHeight;
  computeCenteredImagePlacement(dims.width, dims.height, pageWidth, pageHeight, x, y, renderWidth, renderHeight);

-  GUI.fillPopupProgress(renderer, popupRect, 50);
-
  RenderConfig config{};
  config.x = x;
  config.y = y;