A little bit of feedback

On Sun, Apr 18, 2021 at 08:32:20AM +0200, Steve Lhomme wrote:

> There's a lot in this thread. But long story short, the library was created
> in 2002 as a proof of concept for the format. Then things evolved
> organically from there, keeping the temporary names of that time. And
> because one doesn't break API just like that things have remained in that
> state, with the occasional bug/security fix.
>
> One imporant aspect here is "if it ain't broken, don't fix it". I think
> changing things radically would do more harm than good, at least in the
> short term. It may not looked like it but the library is in a stable state
> and has been fuzzed. Going backward is not an option, unless one wants to
> rewrite mkvtoolnix or the (big) VLC module.
>
> Changing one line of code in libebml/libmatroska may be benign but it may
> involve changing 100 lines in 2 other projects (and maybe more). It's not as
> simple as it looks.
>
> I did start writing a different library for mkclean/mkvalidator, in C.
> Mostly because at the time some of my targets didn't have a C++ compiler. If
> you use libmatroska2 you know what I'm talking about. But you also notice it
> was never released as a standalone library. I don't think it's mature/stable
> enough to do so. And that would involve a lot of support/maintaining I don't
> have the time to deal with.
>
> We do have things tagged as ABI or API changes for libebml/libmatroska.
> These changes will come eventually. But that will probably involve a
> coordinated effort between our libs, mkvtoolnix (Moritz) and VLC (myself).
> And even then, that will mean supporting 2 different pieces for an
> indefinite time, until the supported distros ship the new version. This is
> not a very good incentive to update to a newer library.

That's an awful lot of rather important and elucidating information to
hide in the depths of a mailing list thread.  Are the fuzzing results
available somewhere?

But most importantly nothing I have read here so far in this thread
tells me that what I'm asking for is unreasonable or undoable.
Maybe it would help to give examples of what I would like to see
changed:

* EbmlMaster, EbmlSInteger and EbmlBinary currently derive from
  EbmlElement directly.  Adding an intermediate class EbmlLeaf would
  be a trivial change that does not affect behaviour, but it would
  make writing algorithms massively easier since you would no longer
  have to write Is<EbmlElement>(e) && !Is<EbmlMaster>(e) to perform
  something only on non-masters since you could now use templates
  instantiated on EbmlLeaf.

* EbmlBinary and EbmlSInteger are unalike in that the latter is small
  and will always fit into memory.  With an intermediate class pair
  EbmlLarge/EbmlSmall deriving from EbmlLeaf it would be much easier
  to look into writing abstractions which only hold the small stuff in
  memory and fetch the bulk data from disk on demand.  This, too,
  would be a trivial change that leaves overall behaviour unaffected.

* KaxInternalBlock and KaxBlockBlob are the classes that almost broke
  my brain.  As far as I understand it KaxBlockBlob is supposed to be
  a superclass to KaxBlockGroup and KaxSimpleBlock, and
  KaxInternalBlock is supposed to hold members and methods common to
  them, but it is not supposed to be used on its own.

  But I also see that vlc/modules/demux/mkv/mkv.cpp of the 4.0.0-dev
  repo actually uses a KaxInternalBlock& reference by statically
  casting from KaxBlock or KaxSimpleBlock, but that appears to have
  been added only "recently" (commit:
  5feb3a0476643324f8e418c89abb44b61cf75cae).  That treats
  KaxInternalBlock like an interface/superclass for KaxBlock and
  KaxSimpleBlock instead of as a building block used in class
  composition.  Doing it this way is wrong IMO, KaxInternalBlock
  should not be part of the exposed API.  Otherwise comments should
  explain the details.

  If instead KaxBlockBlob (which should actually be called
  KaxBlocklike) were written as an interface (maybe deriving from
  EbmlElement) which KaxBlockGroup and KaxSimpleBlock merely implement
  using methods and members from KaxInternalBlock (which should really
  be called KaxBlockRump) it would just express what is desired
  (accessing a block-like element from a Cluster regardless of whether
  it's an EbmlElement or EbmlMaster under the hood) and it would
  actually become easy to understand.  A user could then interact with
  KaxSimpleBlock and KaxBlockGroup instances using the KaxBlockBlob
  interface (e.g. get access to the underlying DataBuffers) when they
  don't care about whether it's an EbmlMaster or an EbmlBinary, and
  otherwise use what KaxSimpleBlock and KaxBlockGroup offer specific
  to themselves.

  (MKVToolNix doesn't use KaxInternalBlock anywhere.)

* KaxInternalBlock is quite large -- neither KaxBlock nor
  KaxBlockGroup add much what's not in there already.  I suspect that
  is because this was the original KaxBlock class, and it was merely
  renamed, but not cut apart.  If true that'd be bad because it makes
  instances of KaxSimpleBlock bigger than they need to be which
  sabotages the reason they were introduced in the first place
  (reduced overhead).  Moving stuff not common to both KaxSimpleBlock
  and KaxBlock to KaxBlock/KaxBlockGroup would result in a smaller
  memory footprint without changing behaviour.  This, too, should be
  easy.

And many other such rearranging changes.  None of this would result in
any different behaviour or break stuff, but it would make everything
easier to understand and reason about algorithmically.  Also, if the
test suite were in a working state then it could be grown to test for
behavioural changes, and all of this would become much easier to
tackle.

One more thing about renaming: there are numerous ways in C++ to
provide aliases for many things, so it should at least be up for
discussion for cases where offering one is possible.  C++14 even
supports aliasing function templates.

It'd be really nice if the discussion could move from "Refactoring is
out of question for these reasons" to "Refactoring is okay under the
following conditions".  At least then work towards improvement could
finally commence, however slowly.

Regards,
Dennis.
_______________________________________________
Matroska-devel mailing list
[email protected]
https://lists.matroska.org/cgi-bin/mailman/listinfo/matroska-devel
Read Matroska-Devel on GMane: http://dir.gmane.org/gmane.comp.multimedia.matroska.devel