| Message ID | 20181128161524.96915-1-kjeyapal@akamai.com |
|---|---|
| State | New |
| Headers | show |
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 [...]
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
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 [...]
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
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)