[FFmpeg-devel,v2] avformat/movenc : Don't write sidx for empty urls

Submitted by kjeyapal@akamai.com on Nov. 28, 2018, 4:15 p.m.

Details

Message ID 20181128161524.96915-1-kjeyapal@akamai.com
State New
Headers show

Commit Message

kjeyapal@akamai.com Nov. 28, 2018, 4:15 p.m.
When movenc is used by other segmenting muxers such as dashenc, url field is always empty.
In such cases it is better to not write sidx, instead of throwing errors.
---
 libavformat/movenc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Michael Niedermayer Nov. 29, 2018, 5:38 p.m.
On Wed, Nov 28, 2018 at 09:45:24PM +0530, Karthick J wrote:
> When movenc is used by other segmenting muxers such as dashenc, url field is always empty.
> In such cases it is better to not write sidx, instead of throwing errors.
> ---
>  libavformat/movenc.c | 3 +++
>  1 file changed, 3 insertions(+)

please correct me if i misunderstand but
dashenc enables global sidx, skiping writing the sidx then always seems a bit
odd

[...]
kjeyapal@akamai.com Nov. 30, 2018, 5:52 a.m.
On 11/29/18 11:08 PM, Michael Niedermayer wrote:
> On Wed, Nov 28, 2018 at 09:45:24PM +0530, Karthick J wrote:

>> When movenc is used by other segmenting muxers such as dashenc, url field is always empty.

>> In such cases it is better to not write sidx, instead of throwing errors.

>> ---

>>  libavformat/movenc.c | 3 +++

>>  1 file changed, 3 insertions(+)

>

> please correct me if i misunderstand but

> dashenc enables global sidx, skiping writing the sidx then always seems a bit

> odd

Your understanding is correct. 
But the motive of dashenc is to disable writing of sidx atom for every moof atom (Let's call this as local sidx for the sake of discussion).
We wanted to disable local sidx as it was adding significant bitrate overhead for chunked streaming(where each frame is a moof).
To disable local sidx we enabled global sidx. And global sidx was not writing any sidx at all from dashenc due to lack of url. 
This behavior of not writing any sidx at all is also acceptable for dash. But we just wanted to remove the error, and hence this patch.
Maybe one could add a new option no_sidx in movenc for this functionality. But since global_sidx was already achieving the same functionality new option seemed a bit redundant.
>

> [...]

>

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Nov. 30, 2018, 6:31 p.m.
On Fri, Nov 30, 2018 at 05:52:35AM +0000, Jeyapal, Karthick wrote:
> 
> On 11/29/18 11:08 PM, Michael Niedermayer wrote:
> > On Wed, Nov 28, 2018 at 09:45:24PM +0530, Karthick J wrote:
> >> When movenc is used by other segmenting muxers such as dashenc, url field is always empty.
> >> In such cases it is better to not write sidx, instead of throwing errors.
> >> ---
> >>  libavformat/movenc.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >
> > please correct me if i misunderstand but
> > dashenc enables global sidx, skiping writing the sidx then always seems a bit
> > odd
> Your understanding is correct. 
> But the motive of dashenc is to disable writing of sidx atom for every moof atom (Let's call this as local sidx for the sake of discussion).
> We wanted to disable local sidx as it was adding significant bitrate overhead for chunked streaming(where each frame is a moof).
> To disable local sidx we enabled global sidx. And global sidx was not writing any sidx at all from dashenc due to lack of url. 
> This behavior of not writing any sidx at all is also acceptable for dash. But we just wanted to remove the error, and hence this patch.
> Maybe one could add a new option no_sidx in movenc for this functionality. But since global_sidx was already achieving the same functionality new option seemed a bit redundant.

I have no real oppinion on how to solve it, adding a more specific option
could be done.
but enabling global_sidx and then not doing it silently seems quite hackish
as the intended behavior.
I mean please implement this cleanly, whichever way makes most sense.

thanks

[...]
kjeyapal@akamai.com Dec. 4, 2018, 8:45 a.m.
On 12/1/18 12:01 AM, Michael Niedermayer wrote:
> On Fri, Nov 30, 2018 at 05:52:35AM +0000, Jeyapal, Karthick wrote:

> > 

> > On 11/29/18 11:08 PM, Michael Niedermayer wrote:

> > > On Wed, Nov 28, 2018 at 09:45:24PM +0530, Karthick J wrote:

> > >> When movenc is used by other segmenting muxers such as dashenc, url field is always empty.

> > >> In such cases it is better to not write sidx, instead of throwing errors.

> > >> ---

> > >>  libavformat/movenc.c | 3 +++

> > >>  1 file changed, 3 insertions(+)

> > >

> > > please correct me if i misunderstand but

> > > dashenc enables global sidx, skiping writing the sidx then always seems a bit

> > > odd

> > Your understanding is correct. 

> > But the motive of dashenc is to disable writing of sidx atom for every moof atom (Let's call this as local sidx for the sake of discussion).

> > We wanted to disable local sidx as it was adding significant bitrate overhead for chunked streaming(where each frame is a moof).

> > To disable local sidx we enabled global sidx. And global sidx was not writing any sidx at all from dashenc due to lack of url. 

> > This behavior of not writing any sidx at all is also acceptable for dash. But we just wanted to remove the error, and hence this patch.

> > Maybe one could add a new option no_sidx in movenc for this functionality. But since global_sidx was already achieving the same functionality new option seemed a bit redundant.

>

> I have no real oppinion on how to solve it, adding a more specific option

> could be done.

> but enabling global_sidx and then not doing it silently seems quite hackish

> as the intended behavior.

> I mean please implement this cleanly, whichever way makes most sense.

Thanks for your comments. I have submitted a patch with a specific option to disable sidx as you suggested.
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-December/237121.html

regards,
Karthick
>

> thanks

>

> [...]

>

> -- 

> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

>

> Observe your enemies, for they first find out your faults. -- Antisthenes

Patch hide | download patch | download mbox

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 6dab5193b0..d47ecc65ca 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -6706,6 +6706,9 @@  static int mov_write_trailer(AVFormatContext *s)
            mov->tracks[i].data_offset = 0;
         if (mov->flags & FF_MOV_FLAG_GLOBAL_SIDX) {
             int64_t end;
+            // If url is an empty string("") don't write sidx atom.
+            if (s->url[0] == '\0')
+                return 0;
             av_log(s, AV_LOG_INFO, "Starting second pass: inserting sidx atoms\n");
             res = shift_data(s);
             if (res < 0)