| Message ID | 20161019032043.4912-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | cc71fa319fd7edc54c02a1c552a72cebd0fc4287 |
| Headers | show |
Hi James, > On Oct 18, 2016, at 11:20 PM, James Almer <jamrial@gmail.com> wrote: > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/matroskaenc.c | 6 ++++-- > tests/fate/matroska.mak | 2 +- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 6f2957c..03d5326 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -1192,8 +1192,10 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, > av_log(s, AV_LOG_ERROR, "Overflow in display width\n"); > return AVERROR(EINVAL); > } > - put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , d_width / display_width_div); > - put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); > + if (d_width != par->width || display_width_div != 1 || display_height_div != 1) { > + put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , d_width / display_width_div); > + put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); > + } > } else if (display_width_div != 1 || display_height_div != 1) { > put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , par->width / display_width_div); > put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); You could also make the storage simpler by considering whether or not to write DisplayWidth and DisplayHeight independently. For instance: PixelWidth=720 PixelHeight=480 DisplayWidth=640 DisplayHeight=480 could be simplified to this (as DisplayHeight if unstored, defaults to PixelHeight) PixelWidth=720 PixelHeight=480 DisplayWidth=640 However I'm worried that this may break things when #4489 and/or #5301 are resolved. As the DisplayWidth/Height are intended to be used after cropping. In the attached example on #5301 it has: PixelWidth=320 PixelHeight=240 PixelCropBottom=40 PixelCropTop=0 PixelCropLeft=240 PixelCropRight=40 DisplayWidth=320 DisplayHeight=240 If this was remuxed with your patch AND a potential patch to pass PixelCrop elements from source mkv to output mkv, then the result would be: PixelWidth=320 PixelHeight=240 PixelCropBottom=40 PixelCropTop=0 PixelCropLeft=240 PixelCropRight=40 and since the Display Elements are not stored, they would be evaluated as DisplayWidth as "PixelWidth - PixelCropLeft - PixelCropRight" DisplayHeight = "PixelHeight - PixelCropTop - PixelCropBottom" which is DisplayWidth=40 DisplayHeight=200 which is wrong, since the original file intended to show the cropped 40x200 image in a 320x240 display. Dave Rice
Le nonidi 29 vendémiaire, an CCXXV, Dave Rice a écrit : > You could also make the storage simpler by considering whether or not to > write DisplayWidth and DisplayHeight independently. For instance: > PixelWidth=720 > PixelHeight=480 > DisplayWidth=640 > DisplayHeight=480 > > could be simplified to this (as DisplayHeight if unstored, defaults to PixelHeight) > PixelWidth=720 > PixelHeight=480 > DisplayWidth=640 Even apart from the drawbacks you expect, I do not think this micro-optimization is very interesting. Logically, width and height go together, using half a default value seems unnatural. Regards,
On 10/20/2016 2:05 PM, Nicolas George wrote: > Le nonidi 29 vendémiaire, an CCXXV, Dave Rice a écrit : >> You could also make the storage simpler by considering whether or not to >> write DisplayWidth and DisplayHeight independently. For instance: >> PixelWidth=720 >> PixelHeight=480 >> DisplayWidth=640 >> DisplayHeight=480 >> >> could be simplified to this (as DisplayHeight if unstored, defaults to PixelHeight) >> PixelWidth=720 >> PixelHeight=480 >> DisplayWidth=640 > > Even apart from the drawbacks you expect, I do not think this > micro-optimization is very interesting. Logically, width and height go > together, using half a default value seems unnatural. > > Regards, I agree. Pushed as is. Thanks.
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 6f2957c..03d5326 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -1192,8 +1192,10 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv, av_log(s, AV_LOG_ERROR, "Overflow in display width\n"); return AVERROR(EINVAL); } - put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , d_width / display_width_div); - put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); + if (d_width != par->width || display_width_div != 1 || display_height_div != 1) { + put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , d_width / display_width_div); + put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); + } } else if (display_width_div != 1 || display_height_div != 1) { put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYWIDTH , par->width / display_width_div); put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYHEIGHT, par->height / display_height_div); diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak index 63a4252..35ed41f 100644 --- a/tests/fate/matroska.mak +++ b/tests/fate/matroska.mak @@ -4,6 +4,6 @@ FATE_MATROSKA-$(call DEMMUX, MATROSKA, MATROSKA) += fate-matroska-remux fate-matroska-remux: CMD = md5 -i $(TARGET_SAMPLES)/vp9-test-vectors/vp90-2-2pass-akiyo.webm -color_trc 4 -c:v copy -fflags +bitexact -strict -2 -f matroska fate-matroska-remux: CMP = oneline -fate-matroska-remux: REF = 84e950f59677e306f944fca484888c5d +fate-matroska-remux: REF = 9b8398b42804ba12c39d2f47299a0996 FATE_SAMPLES_AVCONV += $(FATE_MATROSKA-yes)
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/matroskaenc.c | 6 ++++-- tests/fate/matroska.mak | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-)