| Message ID | 20200414014610.30933-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | f0d712d0f95ba757943cc6d49a8caa2e1adfb6e7 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/matroskaenc: Don't write elements with their default value | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 4/13/2020 10:46 PM, Andreas Rheinhardt wrote: > This has happened when writing chapters: Both editions as well as > chapters are by default not hidden and given that we don't support > writing hidden chapters at all, we don't need to write said elements at > all. The same goes for ChapterFlagEnabled. LGTM. I assume they were added because they are both listed as Mandatory, despite having a default value, so it may have been confusing and contradictory back then. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This is supposed to get applied before the cosmetics patch. > > libavformat/matroskaenc.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index d0a02c0f5d..d3256d8f5d 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -1400,7 +1400,6 @@ static int mkv_write_chapters(AVFormatContext *s) > editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0); > if (mkv->mode != MODE_WEBM) { > put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGDEFAULT, 1); > - put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGHIDDEN , 0); > } > for (i = 0; i < s->nb_chapters; i++) { > ebml_master chapteratom, chapterdisplay; > @@ -1420,10 +1419,6 @@ static int mkv_write_chapters(AVFormatContext *s) > (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset); > put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart); > put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend); > - if (mkv->mode != MODE_WEBM) { > - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGHIDDEN , 0); > - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGENABLED, 1); > - } > if ((t = av_dict_get(c->metadata, "title", NULL, 0))) { > chapterdisplay = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERDISPLAY, 0); > put_ebml_string(dyn_cp, MATROSKA_ID_CHAPSTRING, t->value); >
James Almer: > On 4/13/2020 10:46 PM, Andreas Rheinhardt wrote: >> This has happened when writing chapters: Both editions as well as >> chapters are by default not hidden and given that we don't support >> writing hidden chapters at all, we don't need to write said elements at >> all. The same goes for ChapterFlagEnabled. > > LGTM. I assume they were added because they are both listed as > Mandatory, despite having a default value, so it may have been confusing > and contradictory back then. > Applied, thanks. - Andreas
LGTM. In general it would be nice if this was "automatic". In other words when writing any element the value is automatically checked against the default value. Maybe using a macro that would also check the default value like this PUT_EBML_VALUE(cp, EDITIONFLAGHIDDEN, value) doing : if (value != MATROSKA_DEFAULT_EDITIONFLAGHIDDEN) put_ebml_uint(cp, MATROSKA_ID_EDITIONFLAGHIDDEN, value) even better : #ifdef MATROSKA_DEFAULT_EDITIONFLAGHIDDEN if (value != MATROSKA_DEFAULT_EDITIONFLAGHIDDEN) #endif put_ebml_uint(cp, MATROSKA_ID_EDITIONFLAGHIDDEN, value) I could easily generate all the default values from the specs. > On April 14, 2020 3:46 AM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > > > This has happened when writing chapters: Both editions as well as > chapters are by default not hidden and given that we don't support > writing hidden chapters at all, we don't need to write said elements at > all. The same goes for ChapterFlagEnabled. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This is supposed to get applied before the cosmetics patch. > > libavformat/matroskaenc.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index d0a02c0f5d..d3256d8f5d 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -1400,7 +1400,6 @@ static int mkv_write_chapters(AVFormatContext *s) > editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0); > if (mkv->mode != MODE_WEBM) { > put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGDEFAULT, 1); > - put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGHIDDEN , 0); > } > for (i = 0; i < s->nb_chapters; i++) { > ebml_master chapteratom, chapterdisplay; > @@ -1420,10 +1419,6 @@ static int mkv_write_chapters(AVFormatContext *s) > (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset); > put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart); > put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend); > - if (mkv->mode != MODE_WEBM) { > - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGHIDDEN , 0); > - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGENABLED, 1); > - } > if ((t = av_dict_get(c->metadata, "title", NULL, 0))) { > chapterdisplay = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERDISPLAY, 0); > put_ebml_string(dyn_cp, MATROSKA_ID_CHAPSTRING, t->value); > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Steve Lhomme: > LGTM. > > In general it would be nice if this was "automatic". In other words when writing any element the value is automatically checked against the default value. Maybe using a macro that would also check the default value like this > > PUT_EBML_VALUE(cp, EDITIONFLAGHIDDEN, value) > > doing : > if (value != MATROSKA_DEFAULT_EDITIONFLAGHIDDEN) > put_ebml_uint(cp, MATROSKA_ID_EDITIONFLAGHIDDEN, value) > > even better : > #ifdef MATROSKA_DEFAULT_EDITIONFLAGHIDDEN > if (value != MATROSKA_DEFAULT_EDITIONFLAGHIDDEN) > #endif > put_ebml_uint(cp, MATROSKA_ID_EDITIONFLAGHIDDEN, value) > > I could easily generate all the default values from the specs. > Yeah, yeah, I know. Our whole writing process is pretty archaic. With this patch I removed all (except one: BlockAddID) elements where we only write the default value; yet in other places where runtime checks were needed they have not been added yet (think of the TrackLanguage). - Andreas >> On April 14, 2020 3:46 AM Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> >> >> This has happened when writing chapters: Both editions as well as >> chapters are by default not hidden and given that we don't support >> writing hidden chapters at all, we don't need to write said elements at >> all. The same goes for ChapterFlagEnabled. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> This is supposed to get applied before the cosmetics patch. >> >> libavformat/matroskaenc.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index d0a02c0f5d..d3256d8f5d 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -1400,7 +1400,6 @@ static int mkv_write_chapters(AVFormatContext *s) >> editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0); >> if (mkv->mode != MODE_WEBM) { >> put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGDEFAULT, 1); >> - put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGHIDDEN , 0); >> } >> for (i = 0; i < s->nb_chapters; i++) { >> ebml_master chapteratom, chapterdisplay; >> @@ -1420,10 +1419,6 @@ static int mkv_write_chapters(AVFormatContext *s) >> (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset); >> put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart); >> put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend); >> - if (mkv->mode != MODE_WEBM) { >> - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGHIDDEN , 0); >> - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGENABLED, 1); >> - } >> if ((t = av_dict_get(c->metadata, "title", NULL, 0))) { >> chapterdisplay = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERDISPLAY, 0); >> put_ebml_string(dyn_cp, MATROSKA_ID_CHAPSTRING, t->value); >> -- >> 2.20.1 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index d0a02c0f5d..d3256d8f5d 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -1400,7 +1400,6 @@ static int mkv_write_chapters(AVFormatContext *s) editionentry = start_ebml_master(dyn_cp, MATROSKA_ID_EDITIONENTRY, 0); if (mkv->mode != MODE_WEBM) { put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGDEFAULT, 1); - put_ebml_uint(dyn_cp, MATROSKA_ID_EDITIONFLAGHIDDEN , 0); } for (i = 0; i < s->nb_chapters; i++) { ebml_master chapteratom, chapterdisplay; @@ -1420,10 +1419,6 @@ static int mkv_write_chapters(AVFormatContext *s) (uint32_t)c->id + (uint64_t)mkv->chapter_id_offset); put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMESTART, chapterstart); put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERTIMEEND, chapterend); - if (mkv->mode != MODE_WEBM) { - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGHIDDEN , 0); - put_ebml_uint(dyn_cp, MATROSKA_ID_CHAPTERFLAGENABLED, 1); - } if ((t = av_dict_get(c->metadata, "title", NULL, 0))) { chapterdisplay = start_ebml_master(dyn_cp, MATROSKA_ID_CHAPTERDISPLAY, 0); put_ebml_string(dyn_cp, MATROSKA_ID_CHAPSTRING, t->value);
This has happened when writing chapters: Both editions as well as chapters are by default not hidden and given that we don't support writing hidden chapters at all, we don't need to write said elements at all. The same goes for ChapterFlagEnabled. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This is supposed to get applied before the cosmetics patch. libavformat/matroskaenc.c | 5 ----- 1 file changed, 5 deletions(-)