Dump is writing errors to stdout

advplyr commented 1 year ago

This is how I was writing custom metadata to m4b files using ffmpeg, which I found out was NOT a good idea. I will outline it here to show how tone dumps the file, I am not expecting tone to correctly parse this.

First writing a custom tag to the sample m4b here

ffmpeg -i .\samples\test_m4b.m4b -metadata ASIN="1337" -movflags use_metadata_tags .\samples\output_m4b.m4b

Ffprobe format output:

ffprobe -i .\samples\output_m4b.m4b -show_format -print_format json

    "format": {
        "filename": ".\\samples\\output_m4b.m4b",
        "nb_streams": 2,
        "nb_programs": 0,
        "format_name": "mov,mp4,m4a,3gp,3g2,mj2",
        "format_long_name": "QuickTime / MOV",
        "start_time": "0.000000",
        "duration": "211.302000",
        "size": "1934997",
        "bit_rate": "73259",
        "probe_score": 100,
        "tags": {
            "minor_version": "0",
            "major_brand": "mp42",
            "compatible_brands": "mp42isomndia",
            "gapless_playback": "1",
            "track": "5",
            "genre": "abs",
            "artist": "advplyr",
            "title": "Test 5",
            "album": "node-tone",
            "comment": "testing out tone metadata",
            "album_artist": "advplyr",
            "composer": "Composer 5",
            "date": "2022-09-10",
            "ASIN": "1337",
            "compilation": "0",
            "media_type": "2",
            "encoder": "Lavf58.73.100"
        }
    }

Now tone dump

tone dump .\samples\output_m4b.m4b --format json

Output:

Unrecognized metadata format
   at ATL.AudioData.IO.MP4.readTag(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.readUserData(BinaryReader source, ReadTagParams readTagParams, Int64 moovPosition, UInt32 moovSize)
   at ATL.AudioData.IO.MP4.readMP4(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.read(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.Read(BinaryReader source, SizeInfo sizeInfo, ReadTagParams readTagParams)
   at ATL.AudioData.AudioDataManager.read(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.AudioDataManager.read(BinaryReader source, Boolean readEmbeddedPictures, Boolean readAllMetaFrames, Boolean prepareForWriting)
   at ATL.AudioData.AudioDataManager.ReadFromFile(Boolean readEmbeddedPictures, Boolean readAllMetaFrames)
Unrecognized metadata format
   at ATL.AudioData.IO.MP4.readTag(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.readUserData(BinaryReader source, ReadTagParams readTagParams, Int64 moovPosition, UInt32 moovSize)
   at ATL.AudioData.IO.MP4.readMP4(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.read(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.IO.MP4.Read(BinaryReader source, SizeInfo sizeInfo, ReadTagParams readTagParams)
   at ATL.AudioData.AudioDataManager.read(BinaryReader source, ReadTagParams readTagParams)
   at ATL.AudioData.AudioDataManager.read(BinaryReader source, Boolean readEmbeddedPictures, Boolean readAllMetaFrames, Boolean prepareForWriting)
   at ATL.AudioData.AudioDataManager.ReadFromFile(Boolean readEmbeddedPictures, Boolean readAllMetaFrames)
{
  "audio": {
    "format": "Unknown",
    "formatShort": "Unknown",
    "channels": {
      "description": "Unknown"
    },
    "frames": {},
    "metaFormat": []
  },
  "meta": {
    "title": "output_m4b"
  },
  "file": {
    "size": 1934997,
    "created": "2022-09-18T16:20:03.2173223-05:00",
    "modified": "2022-09-18T16:21:28.9052672-05:00",
    "accessed": "2022-09-18T16:26:33.0977549-05:00",
    "path": "\\NodeProjects\\node-tone\\samples",
    "name": "output_m4b.m4b"
  }
}

The issue is that tone is outputting all of this to stdout so node-tone is unable parse the JSON of the metadata that was found. It is okay that tone didn't pull the ASIN tag, but I still want to get the rest of the output.

sandreas commented 1 year ago

Ah, this pretty much relates to #25... there should never be an exception printed out, especially when dumping data. Errors have to be handled very carefully and robust.

sandreas commented 1 year ago

So, I tried to follow the steps creating a bogus m4b file via:

ffmpeg -i without_id3.m4b -metadata ASIN="1337" -movflags use_metadata_tags with_id3.m4b

and it failed to create a file. Would you mind providing a test file, that leads to this error, so that I can reproduce the issue directly?

sandreas commented 1 year ago

So now I added a simple try catch block to prevent these errors in stdout, but I have to verify, that this fixes your problem - so an example file would be awesome.

The try catch is targeted for v0.1.1

advplyr commented 1 year ago

I just committed a sample m4b with bad metadata here: https://github.com/advplyr/node-tone/tree/master/samples

tone dump .\samples\m4b_bad_metadata.m4b --format json

Will produce the error

sandreas commented 1 year ago

Thank you, reproduced. I'll try what I can :-)

sandreas commented 1 year ago

Ok, I found a problem in atldotnet - the audio lib of tone. Usually the repo owner is very polite and quick with fixes, so I would like to wait until it is gonna fixed "the right way", but I also already commited a workaround to prevent this. If the lib cannot be fixed in an acceptable time period, I'll go for the workaround in v0.1.1.

advplyr commented 1 year ago

I want to add that when I write ASIN as an additional field in tone to an m4b file it won't show when doing an ffprobe. When I write ASIN to an mp3 file it does show when doing an ffprobe. Is ffprobe not able to parse additional fields for mp4 files?

sandreas commented 1 year ago

Is ffprobe not able to parse additional fields for mp4 files?

I think so. If you have a sample m4b file, where it does show an ASIN, you can analyse it with tone, it is something like this: ----:com.audible:ASIN.

I use ----:com.pilabor.tone:AUDIBLE_ASIN(my own custom specification).

advplyr commented 1 year ago

I can't get an m4b file to show the ASIN in both ffprobe and tone. Tone won't read the ASIN embedded by ffmpeg and ffprobe won't read the ASIN embedded by tone.

sandreas commented 1 year ago

You can add a new issue for this with an example file, if you like. I don't think, that ASIN is a specified Tag in any well known format, but I'm definitely willing to investigate this.

In mp3 / id3 it is probably TXXX (https://community.metabrainz.org/t/aac-and-alac-file-tagging-question/504352/4, https://id3.org/id3v2.3.0#Text_information_frames) or just ignoring the specs.
In mp4 there is officially no specified ATOM for ASIN, so I don't know how ffmpeg solves this

https://gist.github.com/coolaj86/965870/9a0ad076fddd9a2ae0ab4eddc2ab1a3dc222d50a

advplyr commented 1 year ago

I think ffmpeg is writing custom id3 tags into mp4 files which is why this causes problems with tone. I thought this would be the case and wouldn't consider it an issue with tone or ffmpeg just this argument in ffmpeg movflags use_metadata_tags is the problem.

sandreas commented 1 year ago

I think ffmpeg is writing custom id3 tags into mp4 files which is why this causes problems with tone.

If that is the case, ffmpeg in my opinion does the wrong thing.

What you could also try:

ffmpeg -i input.mp3 -movflags use_metadata_tags -metadata ASIN=B01M1KJCQB outfile.m4a

ffmpeg -i input.mp3 -movflags use_metadata_tags -metadata "----:com.apple.iTunes:ASIN=B01M1KJCQB" outfile.m4a

Is the file you provided generated this way? If so, I could talk to the atldotnet expert about what is not ok in this file and if he would be able to ignore this case without acting against every spec :-)

advplyr commented 1 year ago

I'm not sure if ffmpeg is doing the wrong thing or I am just using it incorrectly with that -movflags argument.

I just tested what you mentioned and it causes the same error in tone as I mentioned in the OP. I think it is safe to say that -movflags use_metadata_tags should not be used with mp4 files and this is not a problem with altdotnet/tone.

sandreas commented 1 year ago

I think you might be right... (see https://github.com/mifi/lossless-cut/issues/402).

Basically this is why I wrote tone... because ffmpeg just can't handle some metadata. Therefore, in m4b-tool I manage all metadata by myself including the following steps:

Reading out all metadata of all files to merge (here I use tone but in the past I used a combination of a PHP class and ffmpeg)
Recalculating metadata (track 1 of 10, Disk 1 of 2, sort title, movement-name, etc.)
merging the file with ffmpeg ignoring all source metadata
tagging the file with the recalculated metadata (with tone or mp4tags)

This is a huge effort but it is accurate, while ffmpeg produces bogus files every now and then... (e.g. concat filter produces an empty file, if the listing.txt contains only one file...)

So to conclue this issue, my todo:

[ ] Prevent tone from polluting stdout / stderr with error messages from invalid files
[ ] Return a unique errorcode, if the file could not be dumped or tagged

sandreas commented 1 year ago

Ok, since there was no release of atldotnet yet, I decided to give 0.1.1 a go... Could you please try if this fixed your issue?

Explanation:

exit code now should be 4 on errors while reading metadata
console is no longer polluted with errors
to see error details you can use --debug, --log-level and --log-file (which has to be documented in the near future)

Using --debug should create a logfile in YOURTMPPATH/tone.log or you specify the location e.g. via --log-file=/home/sandreas/tone.log. You can also use --log-level=debug, but the --log-level option is more meant for future usage.

advplyr commented 1 year ago

Is there a way to get an error message in stderr or stdout?

node-tone is always going to be using the json format. I can now correctly identify it failed using the error code but I won't be able to get any error message to the user without it coming through stdout or stderr.

As far as what you mentioned for this release my tests are all working as you described.

sandreas commented 1 year ago

Is there a way to get an error message in stderr or stdout?

You mean putting the error to stderr and the dump output to stdout? Well, that would be possible... I thought putting the error to the logfile on demand (--log-level) would be better, but stderr is also an option :-) I'll check that out and post a test version before the next release.

advplyr commented 1 year ago

With the log file there is no good way for this to be used in abs that I know of.

I'm assuming when an exit code of 4 is returned this is a critical error and I'm going to ignore the stdout. In that case I could log the error from stderr in abs. For this first implementation I'm going to have abs fallback to ffprobe when a tone dump returns exit code > 0.

sandreas commented 1 year ago

Yeah, I would target the stderr output for 0.1.2...

sandreas commented 1 year ago

Should be fixed in the latest code - targeted for 0.1.2

sandreas / tone

Dump is writing errors to stdout #26