[FFmpeg-devel] [PATCH 10/15] avformat/matroskaenc: Avoid seeking when writing level 1 elements

[James Almer]

On 4/2/2019 1:23 PM, Hendrik Leppkes wrote:
> On Tue, Apr 2, 2019 at 5:55 PM Andreas Rheinhardt via ffmpeg-devel
> <ffmpeg-devel at ffmpeg.org> wrote:
>>
>> Hello,
>>
>> thanks for taking a look at this.
>>
>> Hendrik Leppkes:
>>> On Tue, Apr 2, 2019 at 3:36 PM Andreas Rheinhardt via ffmpeg-devel
>>> <ffmpeg-devel at ffmpeg.org> wrote:
>>>>
>>>> Up until now, the writing process for level 1 elements (those elements
>>>> for which CRC-32 elements are written by default) was this in case the
>>>> output was seekable: Write the EBML ID, write an "unkown length" EBML
>>>> number of the desired length, then write the element into a dynamic
>>>> buffer, then write the dynamic buffer (after possible calculation and
>>>> writing of the CRC-element), then seek back to the size element and
>>>> overwrite the unknown-size element with the real size. The seeking and
>>>> overwriting part has been eliminated by not writing the size initially.
>>>>
>>>
>>> I'm not particularly happy that it stops using start_ebml_master and
>>> basically duplicates its functionality. This adds possible maintenance
>>> in the future, or hidden bugs.
>>>
>> 1. start_ebml_master has the disadvantage that the length of the size
>> element is chosen in advance; if one doesn't want to use another
>> dynamic buffer for every master element (I'm thinking about the
>> thousands of cues for which this would be mostly useless as they are
>> written with one byte length fields anyway) and doesn't want to waste
>> bytes for writing lots of elements (the level 1 elements), then these
>> two functions need to be separated.
> 
> I understand why its done, it just doesn't seem .. ideal.
> 
> I didn't check the code entirely, but can this function theoretically
> still be used for non-L1 elements after your changes, if one is
> desired to have a CRC32 further down the EBML tree?
> If not, it might be sensible to rename it.

Any element can have a CRC32 child element, but it's not required and i
don't think it's worth trying. Only Level 1 elements should have one,
and that's what's currently being done.

> 
>>
>> 6. Or do you mean by making "relative positions valid again" that you
>> want that the offset in s->pb + the offset of an element in the
>> dynamic buffer coincides with the position where this element will
>> actually by written in the output? This would effectively mean that
>> one can't use the minimum required number of bytes for the cluster's
>> size field and instead would have to reserve so many that it always
>> works. The only advantage of this would be that the "Writing block at
>> offset" log message can really spit out the real output offset of the
>> block (which is impossible if the length of the cluster's size field
>> hasn't been chosen before writing the block). That's a very slim
>> advantage to me.
>>
> 
> I forgot that relative positions  within an element are supposed to be
> counted only from the size element, so nevermind that part.
> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>