Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-10-01 Thread James Almer 
On 9/26/2018 10:45 AM, Dave Rice wrote:
> 
>> On Sep 12, 2018, at 11:56 AM, Sigríður Regína Sigurþórsdóttir 
>>  wrote:
>>
>> On Thu, Sep 6, 2018 at 3:31 PM James Almer > > wrote:
>>>
>>> On 9/6/2018 4:18 PM, James Darnley wrote:
 On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
> +if (s->metadata_header_padding) {
> +if (s->metadata_header_padding == 1)
> +s->metadata_header_padding++;
> +put_ebml_void(pb, s->metadata_header_padding);
> +}

 Unfortunately I was forced to make the default -1 so you want to check
 that the value is greater than 0 rather than just true.

 Furthermore I think you will still want to add to Changelog making a
 note that the matroska muxer will now listen to metadata_header_padding.
>>>
>>> No, this kind of change doesn't justify a Changelog entry as mentioned
>>> before.
>>>
 That may also want a micro version bump so that library users can check.
>>>
>>> Micro version bump is ok.
>>
>>
>> Thank you.
>>
>> Here is an updated patch with a bump and a change to make sure the value is 
>> > 0.
>>
>>
>>
>> From 08e140fa0b23274a4db18ce0b201e45fe7c1ac97 Mon Sep 17 00:00:00 2001
>> From: Sigga Regina mailto:siggareg...@gmail.com>>
>> Date: Wed, 12 Sep 2018 11:47:47 -0400
>> Subject: [PATCH] avformat/matroskaenc: add reserve free space option
>>
>> ---
>> libavformat/matroskaenc.c | 5 +
>> libavformat/version.h | 2 +-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 09a62e1..3f5febf 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -2005,6 +2005,11 @@ static int mkv_write_header(AVFormatContext *s)
>> ret = AVERROR(ENOMEM);
>> goto fail;
>> }
>> +if (s->metadata_header_padding > 0) {
>> +  if (s->metadata_header_padding == 1)
>> +s->metadata_header_padding++;
>> +  put_ebml_void(pb, s->metadata_header_padding);
>> +}
>> if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) {
>> mkv->cues_pos = avio_tell(pb);
>> if (mkv->reserve_cues_space == 1)
>> diff --git a/libavformat/version.h b/libavformat/version.h
>> index 4d21583..d7a1a35 100644
>> --- a/libavformat/version.h
>> +++ b/libavformat/version.h
>> @@ -33,7 +33,7 @@
>> // Also please add any ticket numbers that you believe might be affected here
>> #define LIBAVFORMAT_VERSION_MAJOR  58
>> #define LIBAVFORMAT_VERSION_MINOR  18
>> -#define LIBAVFORMAT_VERSION_MICRO 100
>> +#define LIBAVFORMAT_VERSION_MICRO 101
>>
>> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>>LIBAVFORMAT_VERSION_MINOR, \
>> -- 
>> 2.10.1 (Apple Git-78)
>> <0001-avformat-matroskaenc-add-reserve-free-space-option 
>> (1).patch>___
> 
> ping on this, as reserving such space in Matroska headers for later edits to 
> the Tracks element would be helpful.
> Dave Rice

Pushed. Sorry for the delay.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-26 Thread Dave Rice 

> On Sep 12, 2018, at 11:56 AM, Sigríður Regína Sigurþórsdóttir 
>  wrote:
> 
> On Thu, Sep 6, 2018 at 3:31 PM James Almer  > wrote:
>> 
>> On 9/6/2018 4:18 PM, James Darnley wrote:
>>> On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
 +if (s->metadata_header_padding) {
 +if (s->metadata_header_padding == 1)
 +s->metadata_header_padding++;
 +put_ebml_void(pb, s->metadata_header_padding);
 +}
>>> 
>>> Unfortunately I was forced to make the default -1 so you want to check
>>> that the value is greater than 0 rather than just true.
>>> 
>>> Furthermore I think you will still want to add to Changelog making a
>>> note that the matroska muxer will now listen to metadata_header_padding.
>> 
>> No, this kind of change doesn't justify a Changelog entry as mentioned
>> before.
>> 
>>> That may also want a micro version bump so that library users can check.
>> 
>> Micro version bump is ok.
> 
> 
> Thank you.
> 
> Here is an updated patch with a bump and a change to make sure the value is > 
> 0.
> 
> 
> 
> From 08e140fa0b23274a4db18ce0b201e45fe7c1ac97 Mon Sep 17 00:00:00 2001
> From: Sigga Regina mailto:siggareg...@gmail.com>>
> Date: Wed, 12 Sep 2018 11:47:47 -0400
> Subject: [PATCH] avformat/matroskaenc: add reserve free space option
> 
> ---
> libavformat/matroskaenc.c | 5 +
> libavformat/version.h | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 09a62e1..3f5febf 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -2005,6 +2005,11 @@ static int mkv_write_header(AVFormatContext *s)
> ret = AVERROR(ENOMEM);
> goto fail;
> }
> +if (s->metadata_header_padding > 0) {
> +  if (s->metadata_header_padding == 1)
> +s->metadata_header_padding++;
> +  put_ebml_void(pb, s->metadata_header_padding);
> +}
> if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) {
> mkv->cues_pos = avio_tell(pb);
> if (mkv->reserve_cues_space == 1)
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 4d21583..d7a1a35 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -33,7 +33,7 @@
> // Also please add any ticket numbers that you believe might be affected here
> #define LIBAVFORMAT_VERSION_MAJOR  58
> #define LIBAVFORMAT_VERSION_MINOR  18
> -#define LIBAVFORMAT_VERSION_MICRO 100
> +#define LIBAVFORMAT_VERSION_MICRO 101
> 
> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>LIBAVFORMAT_VERSION_MINOR, \
> -- 
> 2.10.1 (Apple Git-78)
> <0001-avformat-matroskaenc-add-reserve-free-space-option 
> (1).patch>___

ping on this, as reserving such space in Matroska headers for later edits to 
the Tracks element would be helpful.
Dave Rice


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-12 Thread Sigríður Regína Sigurþórsdóttir 
On Thu, Sep 6, 2018 at 3:31 PM James Almer  wrote:
>
> On 9/6/2018 4:18 PM, James Darnley wrote:
> > On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
> >> +if (s->metadata_header_padding) {
> >> +if (s->metadata_header_padding == 1)
> >> +s->metadata_header_padding++;
> >> +put_ebml_void(pb, s->metadata_header_padding);
> >> +}
> >
> > Unfortunately I was forced to make the default -1 so you want to check
> > that the value is greater than 0 rather than just true.
> >
> > Furthermore I think you will still want to add to Changelog making a
> > note that the matroska muxer will now listen to metadata_header_padding.
>
> No, this kind of change doesn't justify a Changelog entry as mentioned
> before.
>
> >  That may also want a micro version bump so that library users can check.
>
> Micro version bump is ok.


Thank you.

Here is an updated patch with a bump and a change to make sure the value is > 0.



>From 08e140fa0b23274a4db18ce0b201e45fe7c1ac97 Mon Sep 17 00:00:00 2001
From: Sigga Regina 
Date: Wed, 12 Sep 2018 11:47:47 -0400
Subject: [PATCH] avformat/matroskaenc: add reserve free space option

---
 libavformat/matroskaenc.c | 5 +
 libavformat/version.h | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 09a62e1..3f5febf 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2005,6 +2005,11 @@ static int mkv_write_header(AVFormatContext *s)
 ret = AVERROR(ENOMEM);
 goto fail;
 }
+if (s->metadata_header_padding > 0) {
+  if (s->metadata_header_padding == 1)
+s->metadata_header_padding++;
+  put_ebml_void(pb, s->metadata_header_padding);
+}
 if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) {
 mkv->cues_pos = avio_tell(pb);
 if (mkv->reserve_cues_space == 1)
diff --git a/libavformat/version.h b/libavformat/version.h
index 4d21583..d7a1a35 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -33,7 +33,7 @@
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  58
 #define LIBAVFORMAT_VERSION_MINOR  18
-#define LIBAVFORMAT_VERSION_MICRO 100
+#define LIBAVFORMAT_VERSION_MICRO 101

 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
LIBAVFORMAT_VERSION_MINOR, \
-- 
2.10.1 (Apple Git-78)


0001-avformat-matroskaenc-add-reserve-free-space-option (1).patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-06 Thread James Almer 
On 9/6/2018 4:18 PM, James Darnley wrote:
> On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
>> +if (s->metadata_header_padding) {
>> +if (s->metadata_header_padding == 1)
>> +s->metadata_header_padding++;
>> +put_ebml_void(pb, s->metadata_header_padding);
>> +}
> 
> Unfortunately I was forced to make the default -1 so you want to check
> that the value is greater than 0 rather than just true.
> 
> Furthermore I think you will still want to add to Changelog making a
> note that the matroska muxer will now listen to metadata_header_padding.

No, this kind of change doesn't justify a Changelog entry as mentioned
before.

>  That may also want a micro version bump so that library users can check.

Micro version bump is ok.

> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-06 Thread Carl Eugen Hoyos 
2018-09-06 21:18 GMT+02:00, James Darnley :
> On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
>> +if (s->metadata_header_padding) {
>> +if (s->metadata_header_padding == 1)
>> +s->metadata_header_padding++;
>> +put_ebml_void(pb, s->metadata_header_padding);
>> +}
>
> Unfortunately I was forced to make the default -1 so you want to check
> that the value is greater than 0 rather than just true.
>
> Furthermore I think you will still want to add to Changelog making a
> note that the matroska muxer will now listen to metadata_header_padding.

I don't remember a new option that warranted a Changelog entry...

>  That may also want a micro version bump so that library users can check.

Of course!

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-06 Thread James Darnley 
On 2018-09-06 19:39, Sigríður Regína Sigurþórsdóttir wrote:
> +if (s->metadata_header_padding) {
> +if (s->metadata_header_padding == 1)
> +s->metadata_header_padding++;
> +put_ebml_void(pb, s->metadata_header_padding);
> +}

Unfortunately I was forced to make the default -1 so you want to check
that the value is greater than 0 rather than just true.

Furthermore I think you will still want to add to Changelog making a
note that the matroska muxer will now listen to metadata_header_padding.
 That may also want a micro version bump so that library users can check.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-06 Thread Sigríður Regína Sigurþórsdóttir 
On Wed, Sep 5, 2018 at 6:29 PM James Darnley  wrote:
>
> On 2018-09-05 22:52, Sigríður Regína Sigurþórsdóttir wrote:
> > +{"reserve_free_space", "Reserve a given amount of space at the
> > beginning og the file for unspecified purpose."
>
> I added the "metadata_header_padding" global option many years ago.  Can
> you not reuse it for this purpose?  Is it not likely to be "metadata"
> that another software might fill this with?

Thank you for the suggestion. Here is an updated version using the
"metadata_header_padding".


---
 libavformat/matroskaenc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 09a62e1..3d8ec3c 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2005,6 +2005,11 @@ static int mkv_write_header(AVFormatContext *s)
 ret = AVERROR(ENOMEM);
 goto fail;
 }
+if (s->metadata_header_padding) {
+if (s->metadata_header_padding == 1)
+s->metadata_header_padding++;
+put_ebml_void(pb, s->metadata_header_padding);
+}
 if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) {
 mkv->cues_pos = avio_tell(pb);
 if (mkv->reserve_cues_space == 1)
-- 
2.6.2


0001-avformat-matroskaenc-add-reserve-free-space-option.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-05 Thread James Darnley 
On 2018-09-05 22:52, Sigríður Regína Sigurþórsdóttir wrote:
> +{"reserve_free_space", "Reserve a given amount of space at the
> beginning og the file for unspecified purpose."

I added the "metadata_header_padding" global option many years ago.  Can
you not reuse it for this purpose?  Is it not likely to be "metadata"
that another software might fill this with?

Also there is a typo in the bit I quoted.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-05 Thread James Almer 
On 9/5/2018 6:36 PM, Carl Eugen Hoyos wrote:
> 2018-09-05 22:52 GMT+02:00, Sigríður Regína Sigurþórsdóttir
> :
>> ---
>>  Changelog | 1 +
>>  doc/muxers.texi   | 4 
>>  libavformat/matroskaenc.c | 8 
>>  3 files changed, 13 insertions(+)
>>
>> diff --git a/Changelog b/Changelog
>> index 0975fee..689b04c 100644
>> --- a/Changelog
>> +++ b/Changelog
>> @@ -21,6 +21,7 @@ version :
>>  - Brooktree ProSumer video decoder
>>  - MatchWare Screen Capture Codec decoder
>>  - WinCam Motion Video decoder
>> +- Matroska muxer has optional free space for unspecified use at the
>> beginning of the file.
> 
> No need for a Changelog entry.
> 
> In which situation is the new option useful?

Adding a bunch of reserved filler bytes is something done by a lot of
writing applications and meant to be used mainly by retagging
applications to add or replace tags in place without the risk of having
to rewrite the entire file. Hence it being added right after the Tags
element.

Mkvtoolnix adds a big Void element for this purpose by default, for
example. But for libavformat I'd rather have this be optional.


> 
> Carl Eugen
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-05 Thread Carl Eugen Hoyos 
2018-09-05 22:52 GMT+02:00, Sigríður Regína Sigurþórsdóttir
:
> ---
>  Changelog | 1 +
>  doc/muxers.texi   | 4 
>  libavformat/matroskaenc.c | 8 
>  3 files changed, 13 insertions(+)
>
> diff --git a/Changelog b/Changelog
> index 0975fee..689b04c 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -21,6 +21,7 @@ version :
>  - Brooktree ProSumer video decoder
>  - MatchWare Screen Capture Codec decoder
>  - WinCam Motion Video decoder
> +- Matroska muxer has optional free space for unspecified use at the
> beginning of the file.

No need for a Changelog entry.

In which situation is the new option useful?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat/matroskaenc: add reserve free space option

2018-09-05 Thread Sigríður Regína Sigurþórsdóttir 
---
 Changelog | 1 +
 doc/muxers.texi   | 4 
 libavformat/matroskaenc.c | 8 
 3 files changed, 13 insertions(+)

diff --git a/Changelog b/Changelog
index 0975fee..689b04c 100644
--- a/Changelog
+++ b/Changelog
@@ -21,6 +21,7 @@ version :
 - Brooktree ProSumer video decoder
 - MatchWare Screen Capture Codec decoder
 - WinCam Motion Video decoder
+- Matroska muxer has optional free space for unspecified use at the
beginning of the file.


 version 4.0:
diff --git a/doc/muxers.texi b/doc/muxers.texi
index f18543e..72da90e 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -1213,6 +1213,10 @@ ffmpeg -i sample_left_right_clip.mpg -an -c:v
libvpx -metadata stereo_mode=left_
 This muxer supports the following options:

 @table @option
+
+@item reserve_free_space
+Reserve the specified amount of bytes for unspecified purpose within
the file header.
+
 @item reserve_index_space
 By default, this muxer writes the index for seeking (called cues in Matroska
 terms) at the end of the file, because it cannot know in advance how much space
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 09a62e1..2eb65f8 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -144,6 +144,7 @@ typedef struct MatroskaMuxContext {
 int have_attachments;
 int have_video;

+int reserve_free_space;
 int reserve_cues_space;
 int cluster_size_limit;
 int64_t cues_pos;
@@ -2005,6 +2006,12 @@ static int mkv_write_header(AVFormatContext *s)
 ret = AVERROR(ENOMEM);
 goto fail;
 }
+if (mkv->reserve_free_space) {
+if (mkv->reserve_free_space == 1)
+mkv->reserve_free_space++;
+put_ebml_void(pb, mkv->reserve_free_space);
+}
+
 if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && mkv->reserve_cues_space) {
 mkv->cues_pos = avio_tell(pb);
 if (mkv->reserve_cues_space == 1)
@@ -2767,6 +2774,7 @@ static const AVCodecTag additional_subtitle_tags[] = {
 #define OFFSET(x) offsetof(MatroskaMuxContext, x)
 #define FLAGS AV_OPT_FLAG_ENCODING_PARAM
 static const AVOption options[] = {
+{"reserve_free_space", "Reserve a given amount of space at the
beginning og the file for unspecified purpose.",
OFFSET(reserve_free_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0,
INT_MAX,   FLAGS },
 { "reserve_index_space", "Reserve a given amount of space (in
bytes) at the beginning of the file for the index (cues).",
OFFSET(reserve_cues_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0,
INT_MAX,   FLAGS },
 { "cluster_size_limit",  "Store at most the provided amount of
bytes in a cluster. ",
OFFSET(cluster_size_limit), AV_OPT_TYPE_INT  , { .i64 = -1 }, -1,
INT_MAX,   FLAGS },
 { "cluster_time_limit",  "Store at most the provided number of
milliseconds in a cluster.",
OFFSET(cluster_time_limit), AV_OPT_TYPE_INT64, { .i64 = -1 }, -1,
INT64_MAX, FLAGS },
-- 
2.6.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel