search

Zeugma440 / atldotnet

Fully managed, portable and easy-to-use C# library to read and edit audio data and metadata (tags) from various audio formats, playlists and CUE sheets
MIT License
442 stars 60 forks source link

Invalid chapters specification is not corrected / validated #179

Closed sandreas closed 1 year ago

sandreas commented 1 year ago

The problem

Today someone came up with a problem, that tone does not dump chapters correctly / as expected on opus files. After investigating this, I noticed that atldotnet reads the chapters as specified and does not validate or correct them, even if

Is this intended behaviour or just hasn't come to your attention? I quickly wrote a short method to correct / fix some of the problems, but I'm not sure, that this is the right way to go.

Here is the original issue with a file attachment to reproduce the problem: https://github.com/sandreas/tone/issues/33

Zeugma440 commented 1 year ago

Hi @sandreas - happy new year to you !

The general principle I've been following is to alter as little data as possible when passing through ATL.

In our case, as itzexor remarked, there's no functional spec about Vorbis Chapters.

=> We should discuss what is an incorrect metadata and how ATL should react when it finds one.

the chapter start is AFTER the total duration of the file

the chapter start is after the chapter end

That's obviously abnormal. Should we skip that chapter entirely, to the risk of losing that data entirely upon rewriting ? Or flag it as invalid to let the client app (here : tone) decide what's best ?

the chapter start and end are equal

What if the author wanted to "tag" a timecode / place a marker of some sorts without creating a bona fide chapter ?

I'm not certain this is an actual abnormality. Why did you bring that one up ?

sandreas commented 1 year ago

Hi @sandreas - happy new year to you !

Hey, happy new year :-)

The general principle I've been following is to alter as little data as possible when passing through ATL.

Yeah this is a very good principle.

=> We should discuss what is an incorrect metadata and how ATL should react when it finds one.

I thought about this. In my opinion ATL should not touch the incorrect metadata, if it is possible. Reading metadata implies not to touch it and dump it as is. Maybe a Log event should be created to be able to react on this problem in the user interface, but I would not try to fix it in any way. Example: The file does contain invalid metadata, which may be unintentionally modified on changes or something.

That's obviously abnormal. Should we skip that chapter entirely, to the risk of losing that data entirely upon rewriting ? Or flag it as invalid to let the client app (here : tone) decide what's best ?

I'm really not sure... I think the best way would be to DOCUMENT this behaviour and leave everything as is.

Example documentation extension: ATL reads metadata as is wherever possible. That means even invalid metadata is not modified, although it may seem strange, e.g. creating a file with chapters having a start time after the total duration of the file, which is not corrected in any way as long as it does not influence the storage of metadata changes. Note: Files may have chapters with zero length in some metadata formats.

What if the author wanted to "tag" a timecode / place a marker of some sorts without creating a bona fide chapter ?

That is exactly the case here. It is an audiobook consisting of many files. Unfortunately, the metadata extraction only applies to the first file. Obviously, this is the place, where the problem comes from and where it should be fixed, BUT: tone seems to have an issue, where dumping these chapters somehow generates a fake length of each chapter, because in these opus files chapters do not have a valid length. I did not know that this was possible.

I'm not certain this is an actual abnormality. Why did you bring that one up ?

I brought that up because I wanted to have a second opinion. I'm totally with you on that, atldotnet does not need a fix anyway, but I wanted to be sure about that.

I'll try to fix the problem in tone, feel free to update the documentation if you like - maybe to not loose this information about invalid metadata / chapters.

Thank you very much for your effort and keep up the good work.

Zeugma440 commented 1 year ago

Thanks for your suggestions, as always !

Maybe a Log event should be created to be able to react on this problem in the user interface, but I would not try to fix it in any way. Example: The file does contain invalid metadata, which may be unintentionally modified on changes or something.

Done => e19aeb4477f16b16222ee3209386854094cf5dab

I think the best way would be to DOCUMENT this behaviour and leave everything as is.

Done => 3036e7cfee405705012ae295068798b776391d15

sandreas commented 1 year ago

Hehe, thanks for your quick response and fix, as always! Happy to work with you again.