diff mbox

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

Message ID CAPgnUfBJ8o4v5skDa1_VVs24M8xRh2mh3zgJNLV06_iy=jAqtg@mail.gmail.com
State New
Headers show

Commit Message

Sigríður Regína Sigurþórsdóttir Sept. 12, 2018, 3:56 p.m. UTC 
On Thu, Sep 6, 2018 at 3:31 PM James Almer <jamrial@gmail.com> 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 <siggaregina@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(-)

Comments

Dave Rice Sept. 26, 2018, 1:45 p.m. UTC | #1 
> On Sep 12, 2018, at 11:56 AM, Sigríður Regína Sigurþórsdóttir <siggaregina@gmail.com> wrote:
> 
> On Thu, Sep 6, 2018 at 3:31 PM James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> 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 <siggaregina@gmail.com <mailto:siggaregina@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
James Almer Oct. 1, 2018, 5:54 p.m. UTC | #2 
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 <siggaregina@gmail.com> wrote:
>>
>> On Thu, Sep 6, 2018 at 3:31 PM James Almer <jamrial@gmail.com <mailto:jamrial@gmail.com>> 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 <siggaregina@gmail.com <mailto:siggaregina@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.
diff mbox

Patch

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, \