Skip to content

Add OPAtomIndexFooter::GetLargestElementSize() to pre-size ReadFrame buffers#154

Merged
jhursty merged 9 commits into
cinecert:masterfrom
RealAdarsh:feature/buffer-resize-crypt
May 3, 2026
Merged

Add OPAtomIndexFooter::GetLargestElementSize() to pre-size ReadFrame buffers#154
jhursty merged 9 commits into
cinecert:masterfrom
RealAdarsh:feature/buffer-resize-crypt

Conversation

@RealAdarsh
Copy link
Copy Markdown
Contributor

@RealAdarsh RealAdarsh commented Jun 6, 2025

This PR adds a small helper API to let callers size their frame buffer before calling ReadFrame(), avoiding RESULT_SMALLBUF without changing the library’s memory-management behavior.

@RealAdarsh
Copy link
Copy Markdown
Contributor Author

Hi @jhursty

Could you please take a look at this PR? If you think any changes are needed, kindly let me know. I’m happy to address any feedback!

@RealAdarsh
Copy link
Copy Markdown
Contributor Author

Hi @jhursty, just a gentle follow-up on this PR when you have some time. Would really appreciate your feedback whenever it's convenient. Thanks!

@jhursty
Copy link
Copy Markdown
Contributor

jhursty commented Apr 27, 2026

Thank you for the contribution. I have considered this but do not agree that it is a good idea. The reason is that it changes the behavior of the API. Specifically, the established memory management policy is that the library will not consume huge amounts of memory at the command of an input document. It is the caller that should implement automatic memory expansion should that be desired. (This policy may have exceptions, but those are probably bugs.)

If the caller wants automatic memory expansion then similar code can be used where the ReadFrame() method is called on any of the accessor objects.

Alternatively, it might be useful to add a method to OPAtomIndexFooter which returns the largest index delta. This method could be queried before any calls to ReadFrame() and the resulting value then used (after checking for local constraints) to size the buffer. This will ensure that RESULT_SMALLBUF will never be returned for a given file.

- Add GetLargestElementSize() method to OPAtomIndexFooter class
- Allows callers to query maximum buffer size needed before ReadFrame calls
- Revert automatic buffer resizing in Read_EKLV_Packet function
- Restore original RESULT_SMALLBUF behavior for insufficient buffer capacity
- Maintains established memory management policy per maintainer feedback
@RealAdarsh RealAdarsh force-pushed the feature/buffer-resize-crypt branch from e59469a to 5fe51f1 Compare April 27, 2026 23:05
- Remove automatic buffer resizing code completely
- Restore original simple buffer capacity checks
- h__Reader.cpp now matches original cinecert/master state
@RealAdarsh
Copy link
Copy Markdown
Contributor Author

@jhursty Thank you for the detailed feedback! I completely understand your concerns about changing the API behavior and the established memory management policy. You're absolutely right that the library should not automatically consume large amounts of memory.

I've completely reworked this PR based on your suggestion.

I have:

  1. Removed automatic buffer resizing - Restored the original RESULT_SMALLBUF behavior
  2. Added GetLargestElementSize() method to OPAtomIndexFooter - Returns the largest index delta as you suggested
  3. Maintains memory management policy - Caller remains in full control of memory allocation

@RealAdarsh RealAdarsh changed the title Add auto buffer resize for SourceLength in EKLV reading Add OPAtomIndexFooter::GetLargestElementSize() to pre-size ReadFrame buffers Apr 28, 2026
@jhursty
Copy link
Copy Markdown
Contributor

jhursty commented Apr 28, 2026

I may have made a poor choice of word by writing "delta" since the index group contains a property DeltaEntryArray, which is not what we want. What I meant to point to is the StreamOffset property of the IndexTableSegment::IndexEntry object.

The difference between two adjacent IndexEntry::StreamOffset properties n+1 and n is the size of the packet containing the edit unit identified by entry n. See method calc_Bitrate() in as-02-info.cpp or asdcp-info.cpp for an example of iterating the index to determine packet size.

Note that in the calc_Bitrate() examples the intent is to ignore part of the packet and just measure essence bytes. In this case you want the size of the whole packet.

@jhursty
Copy link
Copy Markdown
Contributor

jhursty commented Apr 28, 2026

Getting closer!

  • It would be better if GetLargestElementSize() returned a ui64_t value. This would avoid the silent truncation in the current version and leave to the caller the decision about what do if the result is larger than a ui32_t.

  • Maintaining have_prev adds an extra operation to each loop. Maybe better to change the predicate on line 1331 to last_offset > 0.

  • The current algorithm does not include the length of the last element. This value will be the difference between the position of the following Partition Pack and the value of the last IndexEntry::StreamOffset property.

  • In addition to your other tests, please test with an MXF file having only one edit unit. The result should be equal to the size of the respective element.

Thank you for your contribution!

@RealAdarsh
Copy link
Copy Markdown
Contributor Author

@jhursty Thanks for the detailed feedback! I have addressed your PR comment.

last_offset > 0 predicate** — I tried this and realised it silently drops frame 0's size. ASDCP writers initialise m_StreamOffset = 0 (AS_DCP_internal.h:632) and persist that 0 as StreamOffset[0] (e.g. AS_DCP_JP2K.cpp:1268). With last_offset > 0, the diff at the second iteration is also skipped, so frame 0 is never measured. I kept an explicit have_prev flag for correctness — it's one extra branch per iteration, well predicted after the first.

Last element via partition pack position** — implemented as ThisPartition - m_ECOffset - last_offset. m_ECOffset is populated from m_HeaderPart.BodyOffset when reading via a one-line addition in h__Reader.cpp:150, so the method needs no parameter — GetLargestElementSize()

Last element via partition pack position** — implemented as ThisPartition - m_ECOffset - last_offset. There's a small wiring question: m_ECOffset was previously set only during writing (via SetIndexParamsVBR), so on a footer that was just read from a file, it would be 0 and the last-element calculation would be skipped — including for the single-edit-unit case. The reader already computes the same absolute position at h__Reader.cpp:144 (m_HeaderPart.BodyOffset = m_File-TellPosition()). I added a single line at h__Reader.cpp:150 that propagates that value into the footer:

 m_IndexAccess.m_ECOffset = m_HeaderPart.BodyOffset;

This way GetLargestElementSize() works correctly on both freshly-read and freshly-written footers without needing a parameter or any change at the call site. Happy to move this assignment elsewhere if you'd prefer it lived in OPAtomIndexFooter::InitFromFile instead.

Verification

File Result Cross-check
3279-frame SMPTE OPAtom J2K (real DCP trailer) 1,300,627 bytes × 24 fps × 8 / 1M = 249.72 Mb/s, exact match with asdcp-info -r
1-edit-unit MXF (wrapped from a single J2K codestream) 556,886 bytes j2c essence is 556,866 bytes; delta of 20 bytes = 16-byte UL + 4-byte BER (whole packet, as specified)

@jhursty jhursty merged commit 905d604 into cinecert:master May 3, 2026
5 checks passed
@RealAdarsh RealAdarsh deleted the feature/buffer-resize-crypt branch May 3, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants