diff mbox

[FFmpeg-devel,RFC] lavc/mjpegenc_common: Fix aspect ratio

Message ID CAB0OVGrEwKy_o7o4hQna9TnQrHtnUcyuDaZOXfZ2fBRSeYjE6g@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Dec. 10, 2018, 1:54 a.m. UTC 
Hi!

Reading the specification and Wikipedia, it appears to me that FFmpeg
is writing wrong values as aspect ratio for jfif files.
I hope somebody can prove me wrong!

This would need a slightly more sophisticated update to the decoder.

Please comment, Carl Eugen

Comments

Moritz Barsnick Dec. 10, 2018, 8:29 a.m. UTC | #1 
On Mon, Dec 10, 2018 at 02:54:54 +0100, Carl Eugen Hoyos wrote:
> I hope somebody can prove me wrong!

Not sure, see below.

> This would need a slightly more sophisticated update to the decoder.

Because it is also presumed incorrect?

>          put_bits(p, 16, 0x0102);
>          put_bits(p,  8, 0);              /* units type: 0 - aspect ratio */
> -        put_bits(p, 16, sar.num);
>          put_bits(p, 16, sar.den);
> +        put_bits(p, 16, sar.num);
>          put_bits(p, 8, 0); /* thumbnail width */

If a 640x480 (640 x dimension, 480 y dimension) image has a SAR of 4/3,
4 (or the x dimension) is its numerator, 3 (or the y dimension) is its
denominator.

The spec (and Wikipedia) says:
  units (1 byte) Units for the X and Y densities.
    units = 0: no units, X and Y specify the pixel aspect ratio
    units = 1: X and Y are dots per inch
    units = 2: X and Y are dots per cm
  Xdensity (2 bytes) Horizontal pixel density
  Ydensity (2 bytes) Vertical pixel density

So Xdensity and Ydensity should be (e.g.) 4 and 3 in this case, which
are sar.num and sar.den (judging by the struct's field names),
respectively.

It seems to me that that is what the original implementation does.

What is yours and Ulf's interpretation?

Cheers,
Moritz
Michael Niedermayer Dec. 10, 2018, 11:33 a.m. UTC | #2 
On Mon, Dec 10, 2018 at 02:54:54AM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Reading the specification and Wikipedia, it appears to me that FFmpeg
> is writing wrong values as aspect ratio for jfif files.
> I hope somebody can prove me wrong!
> 
> This would need a slightly more sophisticated update to the decoder.
> 
> Please comment, Carl Eugen
>  mjpegenc_common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 03077e11ffcc0b2faf5a2be46b0cb2ea224eca57  0001-lavc-mjpegenc_common-Fix-aspect-ratio.patch
> From 9c42114da17c20ef6d81d3989b5521eaefc15819 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Mon, 10 Dec 2018 02:50:39 +0100
> Subject: [PATCH] lavc/mjpegenc_common: Fix aspect ratio.
> 
> Reported-by: Ulf Zibis
> ---
>  libavcodec/mjpegenc_common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

if its correct, this requires a bump to the version of libavcodec so
the decoder can detect if a file is from before or after the change
[...]
Michael Niedermayer Dec. 10, 2018, 11:38 a.m. UTC | #3 
On Mon, Dec 10, 2018 at 12:33:52PM +0100, Michael Niedermayer wrote:
> On Mon, Dec 10, 2018 at 02:54:54AM +0100, Carl Eugen Hoyos wrote:
> > Hi!
> > 
> > Reading the specification and Wikipedia, it appears to me that FFmpeg
> > is writing wrong values as aspect ratio for jfif files.
> > I hope somebody can prove me wrong!
> > 
> > This would need a slightly more sophisticated update to the decoder.
> > 
> > Please comment, Carl Eugen
> >  mjpegenc_common.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 03077e11ffcc0b2faf5a2be46b0cb2ea224eca57  0001-lavc-mjpegenc_common-Fix-aspect-ratio.patch
> > From 9c42114da17c20ef6d81d3989b5521eaefc15819 Mon Sep 17 00:00:00 2001
> > From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > Date: Mon, 10 Dec 2018 02:50:39 +0100
> > Subject: [PATCH] lavc/mjpegenc_common: Fix aspect ratio.
> > 
> > Reported-by: Ulf Zibis
> > ---
> >  libavcodec/mjpegenc_common.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> if its correct, this requires a bump to the version of libavcodec so
> the decoder can detect if a file is from before or after the change

also there is related code in libavcodec/vaapi_encode_mjpeg.c

[...]
Carl Eugen Hoyos Dec. 10, 2018, 12:19 p.m. UTC | #4 
2018-12-10 9:29 GMT+01:00, Moritz Barsnick <barsnick@gmx.net>:
> On Mon, Dec 10, 2018 at 02:54:54 +0100, Carl Eugen Hoyos wrote:
>> I hope somebody can prove me wrong!
>
> Not sure, see below.
>
>> This would need a slightly more sophisticated update to the decoder.
>
> Because it is also presumed incorrect?
>
>>          put_bits(p, 16, 0x0102);
>>          put_bits(p,  8, 0);              /* units type: 0 - aspect ratio
>> */
>> -        put_bits(p, 16, sar.num);
>>          put_bits(p, 16, sar.den);
>> +        put_bits(p, 16, sar.num);
>>          put_bits(p, 8, 0); /* thumbnail width */
>
> If a 640x480 (640 x dimension, 480 y dimension) image has a SAR of 4/3,
> 4 (or the x dimension) is its numerator, 3 (or the y dimension) is its
> denominator.
>
> The spec (and Wikipedia) says:
>   units (1 byte) Units for the X and Y densities.
>     units = 0: no units, X and Y specify the pixel aspect ratio
>     units = 1: X and Y are dots per inch
>     units = 2: X and Y are dots per cm
>   Xdensity (2 bytes) Horizontal pixel density
>   Ydensity (2 bytes) Vertical pixel density
>
> So Xdensity and Ydensity should be (e.g.) 4 and 3 in this case, which
> are sar.num and sar.den (judging by the struct's field names),
> respectively.
>
> It seems to me that that is what the original implementation does.

The "density" (unless they intentionally reversed the meaning of the
word) is always the inverse of the aspect ratio.
Did they reverse the meaning in your opinion?
Does it make sense to argue that if the unit is "0", the meaning of the
density changes?

Carl Eugen
Moritz Barsnick Dec. 10, 2018, 4:26 p.m. UTC | #5 
On Mon, Dec 10, 2018 at 13:19:57 +0100, Carl Eugen Hoyos wrote:
> >   units (1 byte) Units for the X and Y densities.
> >     units = 0: no units, X and Y specify the pixel aspect ratio
> >     units = 1: X and Y are dots per inch
> >     units = 2: X and Y are dots per cm
> >   Xdensity (2 bytes) Horizontal pixel density
> >   Ydensity (2 bytes) Vertical pixel density
> The "density" (unless they intentionally reversed the meaning of the
> word) is always the inverse of the aspect ratio.

That's what I was wondering as well.

> Did they reverse the meaning in your opinion?
> Does it make sense to argue that if the unit is "0", the meaning of the
> density changes?

That was my interpretation. It's a density, if units != 0, otherwise
the aspect ratio. (Would a density not correspond to the SAR? I.e. not
make sense with square pixels?)

I was wondering what other implementations do? jpglib, *Magick.
Reference images? I didn't find anything quickly, but one may need to
check closer.

Cheers,
Moritz
Nicolas George Dec. 10, 2018, 4:38 p.m. UTC | #6 
Moritz Barsnick (2018-12-10):
> That was my interpretation. It's a density, if units != 0, otherwise
> the aspect ratio. (Would a density not correspond to the SAR? I.e. not
> make sense with square pixels?)

I did not follow the beginning of the discussion, but just in case it
can help and was not obvious in the first place:

Having a density makes sense even for square pixels to carry the
information of the physical size of the image. It makes sense if the
original has a well-defined physical size, like a scanned document.

Also, the density can give the aspect ratio, but there is a catch: if
the density is 20 px/mm horizontally and 10 px/mm vertically, that means
pixels are (1/20)×(1/10) mm², hence their aspect ratio is 10/20 = 1/2,
not 20/10 = 2/1.

Regards,
Kevin Wheatley Dec. 10, 2018, 4:55 p.m. UTC | #7 
I came across a similar discussion here:

https://github.com/OpenImageIO/oiio/pull/1412
Kevin
Carl Eugen Hoyos Dec. 11, 2018, 2:17 a.m. UTC | #8 
2018-12-10 17:55 GMT+01:00, Kevin Wheatley <kevin.j.wheatley@gmail.com>:
> I came across a similar discussion here:
>
> https://github.com/OpenImageIO/oiio/pull/1412

So the solution is:
We don't fix it because it is broken elsewhere?

Carl Eugen
Kevin Wheatley Dec. 11, 2018, 12:53 p.m. UTC | #9 
I guess it depends if you think that it is better to align with
defacto behaviour (and maybe clarify/correct the specifications) or
follow the specs and have users grumble about not matching the
behaviour of 'applications X, Y and Z', I'm pretty certain Photoshop
won't easily change its behaviour. The other messy option would be to
have a compatibility mode flag.

I don't think any of this is perfect!
Kevin
diff mbox

Patch

From 9c42114da17c20ef6d81d3989b5521eaefc15819 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Mon, 10 Dec 2018 02:50:39 +0100
Subject: [PATCH] lavc/mjpegenc_common: Fix aspect ratio.

Reported-by: Ulf Zibis
---
 libavcodec/mjpegenc_common.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
index 31868c9..1d3ee55 100644
--- a/libavcodec/mjpegenc_common.c
+++ b/libavcodec/mjpegenc_common.c
@@ -187,8 +187,8 @@  static void jpeg_put_comments(AVCodecContext *avctx, PutBitContext *p)
          * released revision. */
         put_bits(p, 16, 0x0102);
         put_bits(p,  8, 0);              /* units type: 0 - aspect ratio */
-        put_bits(p, 16, sar.num);
         put_bits(p, 16, sar.den);
+        put_bits(p, 16, sar.num);
         put_bits(p, 8, 0); /* thumbnail width */
         put_bits(p, 8, 0); /* thumbnail height */
     }
-- 
1.7.10.4