Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dump is writing errors to stdout #26

Closed
advplyr opened this issue Sep 18, 2022 · 20 comments
Closed

Dump is writing errors to stdout #26

advplyr opened this issue Sep 18, 2022 · 20 comments

Comments

@advplyr
Copy link

advplyr commented Sep 18, 2022

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

advplyr commented Sep 24, 2022

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
Copy link
Owner

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

@sandreas
Copy link
Owner

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
Copy link
Author

advplyr commented Sep 25, 2022

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
Copy link
Owner

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
Copy link
Author

advplyr commented Sep 26, 2022

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
Copy link
Owner

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.

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

@advplyr
Copy link
Author

advplyr commented Sep 26, 2022

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
Copy link
Owner

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
Copy link
Author

advplyr commented Sep 26, 2022

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
Copy link
Owner

I think you might be right... (see mifi/lossless-cut#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
Copy link
Owner

sandreas commented Oct 7, 2022

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.

@sandreas sandreas added the awaiting feedback Requested user feedback needed to continue label Oct 7, 2022
@advplyr
Copy link
Author

advplyr commented Oct 7, 2022

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
Copy link
Owner

sandreas commented Oct 8, 2022

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
Copy link
Author

advplyr commented Oct 8, 2022

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
Copy link
Owner

sandreas commented Oct 8, 2022

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

@sandreas sandreas removed the awaiting feedback Requested user feedback needed to continue label Oct 10, 2022
@sandreas
Copy link
Owner

Should be fixed in the latest code - targeted for 0.1.2

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

No branches or pull requests

2 participants