quietvoid / dovi_tool

dovi_tool is a CLI tool combining multiple utilities for working with Dolby Vision.
MIT License
578 stars 57 forks source link

Request: Extend --crop to accept crop values #281

Closed Dendraspis closed 5 months ago

Dendraspis commented 5 months ago

Hello quietvoid,

using the --crop/-c parameter activates an auto cropping process: ffmpeg.exe [..] | dovi_tool.exe -m 2 -c extract-rpu - -o "RPU.bin"

It would be nice if the cropping could also be done based on user values, preferably provided as parameter values to the --crop/-c parameter. For example: ffmpeg.exe [..] | dovi_tool.exe -m 2 -c 0,0,276,276 extract-rpu - -o "RPU.bin"

Currently there is a way to extract the data and then use the Editor possibilities to edit the file, but in case you just want to crop it by specific amounts, creating a formatted file with the values, to run the Editor, is a "little" bit unhandy - hope you agree. 😅

hyllm commented 5 months ago

nope... not unhandy... you want to edit, use editor.... crop is simple clearing the active offset values, for injecting into cropped encoded videos... so offset will always be 0,0,0,0

if you encode with black bars, and you need crop values, just don't use crop parameter and use original one.. otherwise, edit.

Dendraspis commented 5 months ago

nope... not unhandy... you want to edit, use editor.

Why so impolite? It would be very helpful also to be able to set the crop based on your cropping, directly, without he need to create a formatted file just for that purpose especially when there is already a --crop parameter.

crop is simple clearing the active offset values

If that's the case, why is the generated RPU file using --crop bigger then the one with those values? In my case 287,846 Bytes for the cropped and 284,966 Bytes for the uncropped RPU file.

for injecting into cropped encoded videos... so offset will always be 0,0,0,0 if you encode with black bars, and you need crop values, just don't use crop parameter and use original one.. otherwise, edit.

You imply that cropping always means "remove the black bars according to the RPU data"? What if you want to crop more?

quietvoid commented 5 months ago

--crop is and was always meant to only be a simple convenience flag. It doesn't take values for a reason.

It would be nice if the cropping could also be done based on user values, preferably provided as parameter values to the --crop/-c parameter.

As mentioned by @hyllm, this doesn't make sense if your intention is to add "extra" crop. The metadata should always have either:

  1. Accurate offsets for the picture active area. In most cases, the original metadata is already correct. If it isn't, you can use the editor to fix it.
  2. No offsets when the picture has no letterboxing/pillarboxing. This is what using --crop results in, nothing more.

I wouldn't support a case where one crops partially and wants to use --crop.

If that's the case, why is the generated RPU file using --crop bigger then the one with those values? In my case 287,846 Bytes for the cropped and 284,966 Bytes for the uncropped RPU file.

The reason is that 3 consecutive 0 bytes are not allowed, so there's an extra byte added for every frame when offsets are 0.

Now for convenience I could consider adding support to input editor configs as string directly, but the active area config is still more complex than just numbers.

Dendraspis commented 5 months ago

Regarding on your answer on #280 and also here, I have to say that I had an extremely different understanding of the --crop parameter and what it does than it actually does. My bad. So I will see how to make the data practicable to be used and, at least meanwhile, use the editor command to alter the RPU files.

Thank you for the clarification! 🙏

The reason is that 3 consecutive 0 bytes are not allowed, so there's an extra byte added for every frame when offsets are 0.

Holy moly, thanks for the clarification as well.

Now for convenience I could consider adding support to input editor configs as string directly, but the active area config is still more complex than just numbers.

If the Editor needs to get edit-based data, then an inline solution might be ever more overkill.... 😕

I will close this issue and think about possible solutions with the commands, that already exist, to make the data practicable to be used when cropping is applied.

quietvoid commented 5 months ago

If the Editor needs to get edit-based data, then an inline solution might be ever more overkill....

No, I'm talking about the --edit-config arg when processing HEVC. It currently takes in a file path but it could also be a JSON string.

Dendraspis commented 5 months ago

That's what I meant, because the command line could then be very long and hard to verify with the eyes. But if you think it is a good idea, feel free to implement it. I mean there might be use cases where it would make processes a lot easier/faster, especially if you plan to create multiple outputs for one source file or just for debug reasons. And at the end every possibility is a good possibility. 😅

Have a nice day and thanks.

quietvoid commented 5 months ago

I was just suggesting it because you said files are not practical.

Dendraspis commented 5 months ago

In my original request it seemed like an overkill, but my intention and understanding of the --crop parameter was different. So the JSON data still had to be created, the only difference would be that no (intermediate) file had to be created in order to feed dovi_tool. The advantage of a file is the logging aspect and the ability to quickly change some values whereas finding the right value in a command line within the Command Prompt could be challenging and increases susceptibility to errors.

So I will stick to the file handling as there will not be an easy and short command/entry for the editor.