Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On 2/26/2015 4:10 PM, Clément Bœsch wrote: Yes, it will return an error and abort the operation. Be it done on the command line or via the API. So said differently it won't work anymore. I do wonder what the point of the whole AVOption API is then, if we can't break it any more than a struct. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 05:15:56PM +0100, Clément Bœsch wrote: On Thu, Feb 26, 2015 at 04:13:10PM +, Derek Buitenhuis wrote: On 2/26/2015 4:10 PM, Clément Bœsch wrote: Yes, it will return an error and abort the operation. Be it done on the command line or via the API. So said differently it won't work anymore. I do wonder what the point of the whole AVOption API is then, if we can't break it any more than a struct. No ABI constraints. ...and user input mapping (string) -- Clément B. pgp_dkgnnyYdS.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 04:13:10PM +, Derek Buitenhuis wrote: On 2/26/2015 4:10 PM, Clément Bœsch wrote: Yes, it will return an error and abort the operation. Be it done on the command line or via the API. So said differently it won't work anymore. I do wonder what the point of the whole AVOption API is then, if we can't break it any more than a struct. No ABI constraints. -- Clément B. pgpVZN2sp9FzL.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 01:53:13PM +, Derek Buitenhuis wrote: On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. Ah, my bad. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. Well, breaking API on purpose when easily avoidable is kind of stupid too. You can also just deprecate the current flag and add yours. The point is to not make the option disappear because it will break callers. Changing the behaviour (enabling by default / making the old option void) is fine, breaking the API (removing the option) not so much. -- Clément B. pgpIwJvMX5kqc.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 4:54 PM, Clément Bœsch u...@pkh.me wrote: On Thu, Feb 26, 2015 at 01:53:13PM +, Derek Buitenhuis wrote: On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. Ah, my bad. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. Well, breaking API on purpose when easily avoidable is kind of stupid too. You can also just deprecate the current flag and add yours. The point is to not make the option disappear because it will break callers. Changing the behaviour (enabling by default / making the old option void) is fine, breaking the API (removing the option) not so much. Its an AVOption, isn't the whole point of this system to have a system that wouldn't break callers when an option is added or removed? How would a caller break? Calling a set operation on a non-existing option won't break, just return an error code. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 05:04:53PM +0100, Hendrik Leppkes wrote: On Thu, Feb 26, 2015 at 4:54 PM, Clément Bœsch u...@pkh.me wrote: On Thu, Feb 26, 2015 at 01:53:13PM +, Derek Buitenhuis wrote: On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. Ah, my bad. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. Well, breaking API on purpose when easily avoidable is kind of stupid too. You can also just deprecate the current flag and add yours. The point is to not make the option disappear because it will break callers. Changing the behaviour (enabling by default / making the old option void) is fine, breaking the API (removing the option) not so much. Its an AVOption, isn't the whole point of this system to have a system that wouldn't break callers when an option is added or removed? How would a caller break? Calling a set operation on a non-existing option won't break, just return an error code. Yes, it will return an error and abort the operation. Be it done on the command line or via the API. So said differently it won't work anymore. -- Clément B. pgpC5H_To78HW.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
This also restricts it to MOV and MP4, since it is only
defined for those formats.
Signed-off-by: Derek Buitenhuis derek.buitenh...@gmail.com
---
libavformat/movenc.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 210f78e..9c6e1be 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1500,7 +1500,8 @@ static int mov_write_pasp_tag(AVIOContext *pb, MOVTrack
*track)
static int mov_write_colr_tag(AVIOContext *pb, MOVTrack *track)
{
-// Ref:
https://developer.apple.com/library/mac/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG9
+// Ref (MOV):
https://developer.apple.com/library/mac/technotes/tn2162/_index.html#//apple_ref/doc/uid/DTS40013070-CH1-TNTAG9
+// Ref (MP4): ISO/IEC 14496-12:2012
if (track-enc-color_primaries == AVCOL_PRI_UNSPECIFIED
track-enc-color_trc == AVCOL_TRC_UNSPECIFIED
@@ -1532,9 +1533,15 @@ static int mov_write_colr_tag(AVIOContext *pb, MOVTrack
*track)
}
}
-avio_wb32(pb, 18);
+/* We should only ever be called by MOV or MP4. */
+av_assert0(track-mode == MODE_MOV || track-mode == MODE_MP4);
+
+avio_wb32(pb, 18 + (track-mode == MODE_MP4));
ffio_wfourcc(pb, colr);
-ffio_wfourcc(pb, nclc);
+if (track-mode == MODE_MP4)
+ffio_wfourcc(pb, nclx);
+else
+ffio_wfourcc(pb, nclc);
switch (track-enc-color_primaries) {
case AVCOL_PRI_BT709: avio_wb16(pb, 1); break;
case AVCOL_PRI_SMPTE170M:
@@ -1555,7 +1562,13 @@ static int mov_write_colr_tag(AVIOContext *pb, MOVTrack
*track)
default: avio_wb16(pb, 2);
}
-return 18;
+if (track-mode == MODE_MP4) {
+int full_range = track-enc-color_range == AVCOL_RANGE_JPEG;
+avio_w8(pb, full_range 7);
+return 19;
+} else {
+return 18;
+}
}
static void find_compressor(char * compressor_name, int len, MOVTrack *track)
@@ -1666,8 +1679,12 @@ static int mov_write_video_tag(AVIOContext *pb,
MOVMuxContext *mov, MOVTrack *tr
if (track-enc-field_order != AV_FIELD_UNKNOWN)
mov_write_fiel_tag(pb, track);
-if (mov-flags FF_MOV_FLAG_WRITE_COLR)
-mov_write_colr_tag(pb, track);
+if (mov-flags FF_MOV_FLAG_WRITE_COLR) {
+if (track-mode == MODE_MOV || track-mode == MODE_MP4)
+mov_write_colr_tag(pb, track);
+else
+av_log(mov-fc, AV_LOG_WARNING, Not writing 'colr' atom. Format
is not MOV or MP4.\n);
+}
if (track-enc-sample_aspect_ratio.den
track-enc-sample_aspect_ratio.num
track-enc-sample_aspect_ratio.den !=
track-enc-sample_aspect_ratio.num) {
--
1.8.3.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 01:47:01PM +, Derek Buitenhuis wrote:
This also restricts it to MOV and MP4, since it is only
defined for those formats.
Signed-off-by: Derek Buitenhuis derek.buitenh...@gmail.com
---
libavformat/movenc.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 210f78e..9c6e1be 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
[...]
+if (mov-flags FF_MOV_FLAG_WRITE_COLR) {
+if (track-mode == MODE_MOV || track-mode == MODE_MP4)
+mov_write_colr_tag(pb, track);
+else
+av_log(mov-fc, AV_LOG_WARNING, Not writing 'colr' atom. Format
is not MOV or MP4.\n);
If the option is set by default, you don't want to warn for no reason.
--
Clément B.
pgpTfpKSHNavO.pgp
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 04:13:10PM +, Derek Buitenhuis wrote: On 2/26/2015 4:10 PM, Clément Bœsch wrote: Yes, it will return an error and abort the operation. Be it done on the command line or via the API. So said differently it won't work anymore. I do wonder what the point of the whole AVOption API is then, if we can't break it any more than a struct. just of the top of my head, i can think of: AVOptions allows listing the available options, like for automatically building a list of available options in a GUI it allows changing the type int-int64, int-double it stores default value, min and max allows access to substructures, one can in some cases move a field to s substructure one could dump and load settings of a struct in a generic way not needing to update the code after fields get added [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The misfortune of the wise is better than the prosperity of the fool. -- Epicurus signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 01:47:01PM +, Derek Buitenhuis wrote: This also restricts it to MOV and MP4, since it is only defined for those formats. Signed-off-by: Derek Buitenhuis derek.buitenh...@gmail.com --- libavformat/movenc.c | 29 +++-- 1 file changed, 23 insertions(+), 6 deletions(-) LGTM (the warning would have to be removed though if the specific codepath would be enabled by default) thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 04:54:02PM +0100, Clément Bœsch wrote: On Thu, Feb 26, 2015 at 01:53:13PM +, Derek Buitenhuis wrote: On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. Ah, my bad. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. Well, breaking API on purpose when easily avoidable is kind of stupid too. You can also just deprecate the current flag and add yours. The point is to not make the option disappear because it will break callers. Changing the behaviour (enabling by default / making the old option void) is fine, breaking the API (removing the option) not so much. the option is in there since a month and was in no release so IMHO if people feel strongly about it then i guess simply replacing the option might be acceptable Strictly correct though it is to deprecate it and make it point to the new option though help text and or warning random idea: the new option could be a int with 3 states, on/force, off, auto where auto would write it only for supported formats like mov and maybe mp4 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On 2/26/2015 5:02 PM, Michael Niedermayer wrote: just of the top of my head, i can think of: AVOptions allows listing the available options, like for automatically building a list of available options in a GUI it allows changing the type int-int64, int-double it stores default value, min and max allows access to substructures, one can in some cases move a field to s substructure one could dump and load settings of a struct in a generic way not needing to update the code after fields get added Well, OK. Still need some reviews of the patch itself though ;). - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] movenc: Write 'colr' box correctly for MP4
On Thu, Feb 26, 2015 at 06:15:30PM +0100, Michael Niedermayer wrote: On Thu, Feb 26, 2015 at 04:54:02PM +0100, Clément Bœsch wrote: On Thu, Feb 26, 2015 at 01:53:13PM +, Derek Buitenhuis wrote: On 2/26/2015 1:50 PM, Clément Bœsch wrote: If the option is set by default, you don't want to warn for no reason. It's not set by default. That patch never went in. Ah, my bad. I don't believe silently not writing it is a valid approach. Then again I think setting a default movflag like that instead of changing the option is also stupid. Well, breaking API on purpose when easily avoidable is kind of stupid too. You can also just deprecate the current flag and add yours. The point is to not make the option disappear because it will break callers. Changing the behaviour (enabling by default / making the old option void) is fine, breaking the API (removing the option) not so much. the option is in there since a month and was in no release so IMHO if people feel strongly about it then i guess simply replacing the option might be acceptable Oh, I completely missed that. Then yeah, sure. Sorry for the noise. [...] -- Clément B. pgpfXU9AATc3g.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
