| Message ID | 20180328160248.11382-1-timo.teras@iki.fi |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 28, 2018 at 07:02:48PM +0300, Timo Teräs wrote: > iTunes currently uses the 'cprt' atom to store the copyright notice > and this patch fixes compatibility with majority of software that > supports the 'ilst' atom. Other software and documentation using this: > - AtomicParseley encodes and parses only 'cprt' > - Most players recognize only 'cprt' > - https://sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html > documents both tag types > - http://mutagen.readthedocs.io/en/latest/api/mp4.html documents > only 'cprt' > > ffmpeg mov reader properly parses the 'cprt' tag inside 'ilst' tag > and functions correctly with streams produced with this commit. > > Since 'cprt' seems to be the current correct atom, it is used by > default. "-movflags legacy_copyright" option is added to revert > back to the old atom type in case that is needed. > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > --- > If the legacy option is not needed, I'm happy to resend patch just > changing the atom types. why dont you store both ? the old should do no harm or am i missing something ? and it avoids the need for the user parameter (which few will find if they run in a file that doesnt work) thx [...]
On Thu, 29 Mar 2018 20:29:44 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Mar 28, 2018 at 07:02:48PM +0300, Timo Teräs wrote: > > iTunes currently uses the 'cprt' atom to store the copyright notice > > and this patch fixes compatibility with majority of software that > > supports the 'ilst' atom. Other software and documentation using > > this: > > - AtomicParseley encodes and parses only 'cprt' > > - Most players recognize only 'cprt' > > - https://sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html > > documents both tag types > > - http://mutagen.readthedocs.io/en/latest/api/mp4.html documents > > only 'cprt' > > > > ffmpeg mov reader properly parses the 'cprt' tag inside 'ilst' tag > > and functions correctly with streams produced with this commit. > > > > Since 'cprt' seems to be the current correct atom, it is used by > > default. "-movflags legacy_copyright" option is added to revert > > back to the old atom type in case that is needed. > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > --- > > If the legacy option is not needed, I'm happy to resend patch just > > changing the atom types. > > why dont you store both ? > the old should do no harm or am i missing something ? > and it avoids the need for the user parameter (which few will find if > they run in a file that doesnt work) AFAIK, 'cprt' is the canonical tag and always has been for iTunes list. I tried to look where the current ffmpeg atom came from, but did not find anything conclusive. It might be copy paste error on ffmpeg from other container variants, and others who supports it in iTunes list is to support ffmpeg generated .movs. While it would do no harm, I'd rather not encode the same data multiple times. And at least for the files I am generating, I would like to not generate the incorrect tag. If the preferred choice is to generate both tags, how about adding option "{skip,disable,omit,no}_itls_cpy" or similar to inhibit it? Timo
On Thu, Mar 29, 2018 at 10:12:50PM +0300, Timo Teras wrote: > On Thu, 29 Mar 2018 20:29:44 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Wed, Mar 28, 2018 at 07:02:48PM +0300, Timo Teräs wrote: > > > iTunes currently uses the 'cprt' atom to store the copyright notice > > > and this patch fixes compatibility with majority of software that > > > supports the 'ilst' atom. Other software and documentation using > > > this: > > > - AtomicParseley encodes and parses only 'cprt' > > > - Most players recognize only 'cprt' > > > - https://sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html > > > documents both tag types > > > - http://mutagen.readthedocs.io/en/latest/api/mp4.html documents > > > only 'cprt' > > > > > > ffmpeg mov reader properly parses the 'cprt' tag inside 'ilst' tag > > > and functions correctly with streams produced with this commit. > > > > > > Since 'cprt' seems to be the current correct atom, it is used by > > > default. "-movflags legacy_copyright" option is added to revert > > > back to the old atom type in case that is needed. > > > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > > --- > > > If the legacy option is not needed, I'm happy to resend patch just > > > changing the atom types. > > > > why dont you store both ? > > the old should do no harm or am i missing something ? > > and it avoids the need for the user parameter (which few will find if > > they run in a file that doesnt work) > > AFAIK, 'cprt' is the canonical tag and always has been for iTunes list. > I tried to look where the current ffmpeg atom came from, but did not > find anything conclusive. It might be copy paste error on ffmpeg from > other container variants, and others who supports it in iTunes list is > to support ffmpeg generated .movs. git log points to bed4fc54c947b9e36d2103b400d438bfb4dd80dd This commit added both cpy and cprt, that makes your hypothesis not fit very well did you ask the author, who added this ? Maybe he remembers where it came from. It would be better to understand where it came from before its changed If it was a typo or something like that, i agree that it should be removed/replaced. I was assuming it come from some official reference/software ... thx [...]
On Fri, Mar 30, 2018 at 3:39 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > > If it was a typo or something like that, i agree that it should be > removed/replaced. I was assuming it come from some official > reference/software ... > For the record, the current QTFF (aka MOV) specification still lists '©cpy' as the copyright statement international text field. The copyright sign at the beginning of the identifier is defined as: "All user data list entries whose type begins with the © character (ASCII 169) are defined to be international text. These list entries must contain a list of text strings with associated language codes. By storing multiple versions of the same text, a single user data text item can contain translations for different languages." Official ref: https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-BBCCFFGD 'cprt' is not mentioned in QTFF. Meanwhile, in ISOBMFF (aka MP4 aka 14496-12, available for free from http://standards.iso.org/ittf/PubliclyAvailableStandards/index.html ) 'cprt' is defined. So with a quick look it seems like when writing in MOV mode one should still utilize '©cpy' while with MP4 one should utilize 'cprt' according to ISOBMFF. Best regards, Jan
On Fri, 30 Mar 2018 02:39:13 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Mar 29, 2018 at 10:12:50PM +0300, Timo Teras wrote: > > On Thu, 29 Mar 2018 20:29:44 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Wed, Mar 28, 2018 at 07:02:48PM +0300, Timo Teräs wrote: > > > > iTunes currently uses the 'cprt' atom to store the copyright > > > > notice and this patch fixes compatibility with majority of > > > > software that supports the 'ilst' atom. Other software and > > > > documentation using this: > > > > - AtomicParseley encodes and parses only 'cprt' > > > > - Most players recognize only 'cprt' > > > > - > > > > https://sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html > > > > documents both tag types > > > > - http://mutagen.readthedocs.io/en/latest/api/mp4.html > > > > documents only 'cprt' > > > > > > > > ffmpeg mov reader properly parses the 'cprt' tag inside 'ilst' > > > > tag and functions correctly with streams produced with this > > > > commit. > > > > > > > > Since 'cprt' seems to be the current correct atom, it is used by > > > > default. "-movflags legacy_copyright" option is added to revert > > > > back to the old atom type in case that is needed. > > > > > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > > > --- > > > > If the legacy option is not needed, I'm happy to resend patch > > > > just changing the atom types. > > > > > > why dont you store both ? > > > the old should do no harm or am i missing something ? > > > and it avoids the need for the user parameter (which few will > > > find if they run in a file that doesnt work) > > > > AFAIK, 'cprt' is the canonical tag and always has been for iTunes > > list. I tried to look where the current ffmpeg atom came from, but > > did not find anything conclusive. It might be copy paste error on > > ffmpeg from other container variants, and others who supports it in > > iTunes list is to support ffmpeg generated .movs. > > git log points to > bed4fc54c947b9e36d2103b400d438bfb4dd80dd > > This commit added both cpy and cprt, that makes your hypothesis not > fit very well That commit adds it to three places: - MOV mode which is basically in mov_write_udta_tag() using '\251cpy' which published in Apple docs - 3GP mode using 'cprt' which is in ISO standard - mov_write_ilst_tag() is iTunes which is non-documented The commit adds exactly one tag to each variant. To me my hyptothesis still looks correct. > did you ask the author, who added this ? Maybe he remembers where it > came from. > It would be better to understand where it came from before its changed @Baptiste: Any comment why \251cpy tag was used for iTunes list and not cprt? > If it was a typo or something like that, i agree that it should be > removed/replaced. I was assuming it come from some official > reference/software ... iTunes list format is unfortunately not documented. MOV and 3GP modes are standardized. Timo
On Fri, 30 Mar 2018 03:54:59 +0300 Jan Ekström <jeebjp@gmail.com> wrote: > On Fri, Mar 30, 2018 at 3:39 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > If it was a typo or something like that, i agree that it should be > > removed/replaced. I was assuming it come from some official > > reference/software ... > > > > For the record, the current QTFF (aka MOV) specification still lists > '©cpy' as the copyright statement international text field. The > copyright sign at the beginning of the identifier is defined as: > "All user data list entries whose type begins with the © character > (ASCII 169) are defined to be international text. These list entries > must contain a list of text strings with associated language codes. By > storing multiple versions of the same text, a single user data text > item can contain translations for different languages." > Official ref: > https://developer.apple.com/library/content/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-BBCCFFGD > > 'cprt' is not mentioned in QTFF. > > Meanwhile, in ISOBMFF (aka MP4 aka 14496-12, available for free from > http://standards.iso.org/ittf/PubliclyAvailableStandards/index.html ) > 'cprt' is defined. > > So with a quick look it seems like when writing in MOV mode one should > still utilize '©cpy' while with MP4 one should utilize 'cprt' > according to ISOBMFF. Correct. And these modes are not touched in the patch, it changes iTunes list writing which is not covered by any standard. Do note that iTunes list is is not MOV format. It's Apple extension to MP4. The MOV mode tag is written in mov_write_udta_tag() in the if mov->mode == MODE_MOV block. The commit bed4fc54c947b9e36d2103b400d438bfb4dd80dd pointed by Michael shows the three code paths writing copyright tag in the different modes/variants. Timo
On Fri, 30 Mar 2018 04:06:03 +0300 Timo Teras <timo.teras@iki.fi> wrote: > On Fri, 30 Mar 2018 02:39:13 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > git log points to > > bed4fc54c947b9e36d2103b400d438bfb4dd80dd > > > > This commit added both cpy and cprt, that makes your hypothesis not > > fit very well > > That commit adds it to three places: > - MOV mode which is basically in mov_write_udta_tag() using '\251cpy' > which published in Apple docs > - 3GP mode using 'cprt' which is in ISO standard > - mov_write_ilst_tag() is iTunes which is non-documented > > The commit adds exactly one tag to each variant. To me my hyptothesis > still looks correct. I was thinking and researching this more. It looks like: - 3GP defines 'cprt' which is completely different encoding and appears under different atom - \251cpy is the QuickTime atom documented by Apple - but it seems iTunes uses and has always used undocumented 'cprt' which similar to other iTunes atoms It seems that other software supporting iTunes support 'cprt' format. There are few other things that support also '\251cpy' variant in this context, but they are few. Now I suspect ffmpeg started writing '\251cpy' atoms also for the iTunes tag based on the assumption that the tag there would follow QuickTime format as many other iTunes tags follow. And this almost sounds like any other software supporting '\251cpy' here do it to support ffmpeg encoded incorrect streams. My preference on fixing this is as follows: 1. Just change '\251cpy' to 'cprt' as this seems to be the canonical atom 2. My previously suggested patch. Write 'cprt' by default, but have flag to write the other tag 3. Write both by default, but add flag to suppress the '\251cpy' atom Suggestions and additional input welcome. Timo
On Mon, Apr 02, 2018 at 12:02:38PM +0300, Timo Teras wrote: > On Fri, 30 Mar 2018 04:06:03 +0300 > Timo Teras <timo.teras@iki.fi> wrote: > > > On Fri, 30 Mar 2018 02:39:13 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > git log points to > > > bed4fc54c947b9e36d2103b400d438bfb4dd80dd > > > > > > This commit added both cpy and cprt, that makes your hypothesis not > > > fit very well > > > > That commit adds it to three places: > > - MOV mode which is basically in mov_write_udta_tag() using '\251cpy' > > which published in Apple docs > > - 3GP mode using 'cprt' which is in ISO standard > > - mov_write_ilst_tag() is iTunes which is non-documented > > > > The commit adds exactly one tag to each variant. To me my hyptothesis > > still looks correct. > > I was thinking and researching this more. It looks like: > - 3GP defines 'cprt' which is completely different encoding and > appears under different atom > - \251cpy is the QuickTime atom documented by Apple > - but it seems iTunes uses and has always used undocumented 'cprt' > which similar to other iTunes atoms > > It seems that other software supporting iTunes support 'cprt' format. > There are few other things that support also '\251cpy' variant in this > context, but they are few. > > Now I suspect ffmpeg started writing '\251cpy' atoms also for the > iTunes tag based on the assumption that the tag there would follow > QuickTime format as many other iTunes tags follow. > > And this almost sounds like any other software supporting '\251cpy' > here do it to support ffmpeg encoded incorrect streams. > > My preference on fixing this is as follows: > 1. Just change '\251cpy' to 'cprt' as this seems to be the canonical > atom > 2. My previously suggested patch. Write 'cprt' by default, but have > flag to write the other tag > 3. Write both by default, but add flag to suppress the '\251cpy' atom > > Suggestions and additional input welcome. if there is no reply from anyone providing more information than the choice you prefer (1.) should be ok thx [...]
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index ef668eccd5..230aeb6a6d 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -79,6 +79,7 @@ static const AVOption options[] = { { "use_metadata_tags", "Use mdta atom for metadata.", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_USE_MDTA}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "skip_trailer", "Skip writing the mfra/tfra/mfro trailer for fragmented files", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_SKIP_TRAILER}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, { "negative_cts_offsets", "Use negative CTS offsets (reducing the need for edit lists)", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_NEGATIVE_CTS_OFFSETS}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, + { "legacy_copyright", "Use legacy iTunes (c)cpy copyright atom inside ilst", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_LEGACY_ILTS_CPRT}, INT_MIN, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "movflags" }, FF_RTP_FLAG_OPTS(MOVMuxContext, rtp_flags), { "skip_iods", "Skip writing iods atom.", offsetof(MOVMuxContext, iods_skip), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM}, { "iods_audio_profile", "iods audio profile atom.", offsetof(MOVMuxContext, iods_audio_profile), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 255, AV_OPT_FLAG_ENCODING_PARAM}, @@ -3432,7 +3433,10 @@ static int mov_write_ilst_tag(AVIOContext *pb, MOVMuxContext *mov, } mov_write_string_metadata(s, pb, "\251cmt", "comment" , 1); mov_write_string_metadata(s, pb, "\251gen", "genre" , 1); - mov_write_string_metadata(s, pb, "\251cpy", "copyright", 1); + if (mov->flags & FF_MOV_FLAG_LEGACY_ILTS_CPRT) + mov_write_string_metadata(s, pb, "\251cpy", "copyright", 1); + else + mov_write_string_metadata(s, pb, "cprt", "copyright", 1); mov_write_string_metadata(s, pb, "\251grp", "grouping" , 1); mov_write_string_metadata(s, pb, "\251lyr", "lyrics" , 1); mov_write_string_metadata(s, pb, "desc", "description",1); diff --git a/libavformat/movenc.h b/libavformat/movenc.h index ca2a9c9722..e83a29aea2 100644 --- a/libavformat/movenc.h +++ b/libavformat/movenc.h @@ -246,6 +246,7 @@ typedef struct MOVMuxContext { #define FF_MOV_FLAG_SKIP_TRAILER (1 << 18) #define FF_MOV_FLAG_NEGATIVE_CTS_OFFSETS (1 << 19) #define FF_MOV_FLAG_FRAG_EVERY_FRAME (1 << 20) +#define FF_MOV_FLAG_LEGACY_ILTS_CPRT (1 << 21) int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt);
iTunes currently uses the 'cprt' atom to store the copyright notice and this patch fixes compatibility with majority of software that supports the 'ilst' atom. Other software and documentation using this: - AtomicParseley encodes and parses only 'cprt' - Most players recognize only 'cprt' - https://sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html documents both tag types - http://mutagen.readthedocs.io/en/latest/api/mp4.html documents only 'cprt' ffmpeg mov reader properly parses the 'cprt' tag inside 'ilst' tag and functions correctly with streams produced with this commit. Since 'cprt' seems to be the current correct atom, it is used by default. "-movflags legacy_copyright" option is added to revert back to the old atom type in case that is needed. Signed-off-by: Timo Teräs <timo.teras@iki.fi> --- If the legacy option is not needed, I'm happy to resend patch just changing the atom types. libavformat/movenc.c | 6 +++++- libavformat/movenc.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-)