[FFmpeg-devel] [PATCH] avformat/matroskaenc: write DisplayWidth and DisplayHeight elements only if they differ from PixelWidth and PixelHeight

[Dave Rice]

Hi James,

> On Oct 18, 2016, at 11:20 PM, James Almer <jamrial at gmail.com> wrote:
> 
> Signed-off-by: James Almer <jamrial at 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