search

MediaArea / BWFMetaEdit

WAV/BWF metadata editor
https://MediaArea.net/BWFMetaEdit
Other
41 stars 18 forks source link

Option to remove specific chunks #281

Open MarcosSueiro opened 5 months ago

MarcosSueiro commented 5 months ago

Follow up to #163

Add option to remove specific chunks: --remove-chunks=[NAME]

JeromeMartinez commented 4 months ago

@MarcosSueiro please test this snapshot with --remove-chunks= option.

MarcosSueiro commented 4 months ago

I will test tomorrow since I am working from home. I am very excited, thank you so much!!

Marcos Sueiro Bal Archive Manager 646 829 4063 From: Jérôme Martinez @.> Sent: Wednesday, February 21, 2024 11:01 AM To: MediaArea/BWFMetaEdit @.> Cc: Marcos Sueiro Bal @.>; Mention @.> Subject: Re: [MediaArea/BWFMetaEdit] Option to remove specific chunks (Issue #281)

@MarcosSueirohttps://github.com/MarcosSueiro please test this snapshothttps://mediaarea.net/download/snapshots/binary/bwfmetaedit/20240220/ with --remove-chunks= option.

— Reply to this email directly, view it on GitHubhttps://github.com/MediaArea/BWFMetaEdit/issues/281#issuecomment-1957087611, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAD7LVD22CY2QHUOOEZTKC3YUYK4VAVCNFSM6AAAAABC4VABZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJXGA4DONRRGE. You are receiving this because you were mentioned.Message ID: @.**@.>>

MarcosSueiro commented 4 months ago

Thank you so much for implementing this! Please see results below.

I tested --remove-chunk= with two files:

  1. File 1 has one 'levl' chunk, 54 'mtyp' and 'labl' subchunks under the 'adtl' chunk, and an 'r64m' chunk
  2. File 2 has one chunk of each of the following labels: 'fact', 'mext', 'levl', 'aux '

image

JeromeMartinez commented 4 months ago

Fifth test: try/catch remove 'adtl' chunk from File 1 --unsuccessful (i.e., BWFME removed a supported chunk)

We don't prevent the removal of supported chunks on purpose, as an easy way to remove this kind of chunks, no difference between supported and unsupported chunks, so IMO it is fine.

Note that we block the removal of mandatory chunks e.g. "fmt "

[...] aux [...]

Works for me with a fake file (I have no real file with "aux "

$ mediainfo --Details=1 a.WAV | grep Name | grep "aux "
0017F8    Name:                                aux
$ bwfmetaedit "--remove-chunks=aux ,levl" a.WAV
$ mediainfo --Details=1 a.WAV | grep Name | grep "aux "
$

Did you think to quote the option in order to provide correctly the space to the program? Note that we don't convert "_" to " ", so if your file has a space you can not use an underscore.

[...] mtyp [...] labl [...]

Tried the same way (without quote) with fake files and it is fine. and weird that this is still in trace but not in table view, did you reload files? Please share non working files.

(needed to reload to see results in GUI)

The GUI does not detect a file change and does not reload, a feature we need to add, another topic.

MarcosSueiro commented 4 months ago

File with multiple mytp and labl subchunks (Jerome only) here

MarcosSueiro commented 4 months ago

We don't prevent the removal of supported chunks on purpose, as an easy way to remove this kind of chunks, no difference between supported and unsupported chunks, so IMO it is fine.

Since this is potentially dangerous, could we add a warning sign before deleting supported chunks?

JeromeMartinez commented 4 months ago

Since this is potentially dangerous, could we add a warning sign before deleting supported chunks?

I have mixed feelings about that, an future issue I see is that a new version of the tool supporting a new chunk item would reject the command, so not responding as the previous version, it seems dangerous to me. Do you think that this potential behavior change between versions is acceptable?

And how dangerous is it, as the user explicitly indicate that this item should be removed?

File with multiple mytp and labl subchunks

I see, they are not at the top level, and currently we support only top level items. cc @g-maxime IMO the interface will be something like "--remove-chunks=cue /adtl/mtyp" in order to avoid conflict with top level chunks with the same name. And another limit is that we won't support sub-chunks of unsupported chunks (technically we could, but more work).

MarcosSueiro commented 4 months ago

I have mixed feelings about that, an future issue I see is that a new version of the tool supporting a new chunk item would reject the command, so not responding as the previous version, it seems dangerous to me. Do you think that this potential behavior change between versions is acceptable?

This is what I see as a potential problem:

Some of our files have (as you have seen) many types of extraneous chunks. Let's say one of the extraneous chunks is called 'adtk', but my finger slips and I write 'adtl' instead of 'adtk': --remove-chunks=levl,adtk,mtyp.

If BWFME does not warn me, I would remove a chunk that I did not intend to delete.

As I see it, we could do two approaches (I think I prefer the first one):

  1. Always generate a warning, as follows: 
    
ATTENTION! You are about to remove the following chunks:
- adtl (** CURRENTLY SUPPORTED in Version 24.01, for Windows **)
- levl (currently unsupported)
- mtyp (currently unsupported)

Please keep in mind that:

  1. Generate a warning only for currently supported chunks: 
    
ATTENTION! You are about to remove the following supported chunks
in BWFME Version 24.01, for Windows:
- adtl

Please keep in mind that:

I see, they are not at the top level, and currently we support only top level items. cc @g-maxime IMO the interface will be something like "--remove-chunks=cue /adtl/mtyp" in order to avoid conflict with top level chunks with the same name. And another limit is that we won't support sub-chunks of unsupported chunks (technically we could, but more work).

I like this solution! And I think supporting sub-chunks of unsupported chunks would be too confusing. 😵‍💫