RandomEngy / VidCoder

A Blu-ray, DVD and video file transcoder for Windows.
http://vidcoder.net
GNU General Public License v2.0
668 stars 42 forks source link

Metadata Pass-through Does Not Copy Comment Field #1193

Closed jlikens closed 1 month ago

jlikens commented 7 months ago

Feature details

One of the core metadata fields in MKV and MP4 containers is the Comments field. Some software uses this field to store structural or informational metadata, such as normalization information, original source material, etc.

Since the data in this field is not passed through to the output file, this data is lost. One possible workaround would be to send the source and destination files to a post-processing script to do the copying, but VidCoder does not seem to allow sending the full path to the these files to the post-encoding script.

So, there are (at least) two possible ways to address the above:

  1. Add the Comment field to the list of metadata that is copied from the source to the destination
  2. The post-encoding arguments should allow for a new replacement tags, such as "{fullsourcepath}" and "{fulldestinationpath}"
    1. While not a direct implementation of the Comment passthrough, it would allow power users a more robust set of parameters for post-processing scripts, including writing something that could do the Comment copying
RandomEngy commented 7 months ago

Do you have Pass through Video Metadata checked in the Picker window?

In any event, I do see that {fullsourcepath} and {fulldestinationpath} would be useful on the post-encoding script, I'll add those when I get some time.

jlikens commented 6 months ago

I do, yes. Here is ffprobe outputs for the metadata tags on a fully-tagged sample MKV, as well as the same ffprobe outputs for the converted file:

Input File Metadata

Input #0, matroska,webm, from 'Mkv Test.mkv':
  Metadata:
    title           : Your Title
    COPYRIGHT       : Copyright Info
    COMPATIBLE_BRANDS: isommp42
    MAJOR_BRAND     : mp42
    MINOR_VERSION   : 0
    PUBLISHER       : Publisher Name
    ARTIST          : Artist Name
    COMPOSER        : Composer Name
    GENRE           : Genre
    DATE            : 2023-12-06
    DESCRIPTION     : Description here
    COMMENT         : Your comment here
    ALBUM           : Album Name
    LANGUAGE        : en
    ENCODER         : Lavf58.76.100

Output File Metadata

Input #0, matroska,webm, from '..\Mkv Test [[Audio Passthru] NVIDIA HEVC 1080p 2500].mkv':
  Metadata:
    title           : Your Title
    DESCRIPTION     : Description here
    creation_time   : 2023-12-06T17:56:46.000000Z
    ARTIST          : Artist Name
    COMPOSER        : Composer Name
    GENRE           : Genre
    ENCODER         : Lavf59.27.100

I grabbed the repo and traced through to where it pulls some of the metadata (not all, due to the way the mapping happens) onto the target encode job. It seems as though most of this is lost when it gets sent to Handbrake, though.

As an aside, I was spinning up a PR for my option 2 above, which was pretty straightforward to implement. However, when trying to spin up a unit test I got snagged up in a pretty hairy ball of hard dependencies (mostly through a lot of StaticResolver calls and some database requests). I wrote up a pathway to start abstracting away some of this (at least for the OutputPathService space). It's a fair amount of refactoring involved to use a more flexible mode of DI, but it can certainly be done.

I don't want to introduce a new design pattern to your codebase, though, so I wasn't planning on actually submitting the PR unless you've got interest there.

RandomEngy commented 6 months ago

For StaticResolver test, you can call StaticResolver.SetResolver() with a custom IResolver that returns whatever you want. But if you'd like to go crazy and switch everything to constructor DI, that's fine with me too. I haven't been the most rigorous with the DI setup since I'm the only one that develops on it most of the time and I don't have a ton of unit tests.

Anyway, happy to look at whatever PR you've got there.

RandomEngy commented 1 month ago

I added a {sourcepath} token to the post-encoding options in 10.9 Beta. The {file} token already is the full destination path.