image.Metadata interface type

[Nigel Tao]

There is a proposal (https://github.com/golang/go/issues/33457) to add
metadata awareness (e.g. EXIF, ICCP, XMP, text comments) to the image
codecs. One aspect is augmenting the existing concrete image types,
like image.RGBA and image.Gray, with a Metadata field. The existing:

type RGBA struct {
Pix []uint8
Stride int
Rect Rectangle
}

would become:

type RGBA struct {
Pix []uint8
Stride int
Rect Rectangle
Metadata M
}

with possibly an additional method (to satisfy a common interface):

func (r *RGBA) GetMetadata() M { return r.Metadata }

for some type M. M would be probably an interface type, with concrete
implementations like gif.Metadata, jpeg.Metadata and png.Metadata,
because it's more flexible, because the underlying file formats don't
really represent their metadata in a uniform way (e.g. not every codec
supports EXIF), and because it would be nice if e.g. `import "image"`
didn't need to depend on "encoding/xml" via XMP metadata support.

My question is, what should that interface type M look like? I have
two possibilities in mind:

1. An empty interface. The field would be "Metadata interface{}".

2. A marker interface. The field would be "Metadata Metadata" and
there'd also be "type Metadata interface { IsMetadata() }". The
IsMetadata method is a no-op, but is an explicit annotation so that
one can search for e.g. all the concrete types that implement
image.Metadata.

Any wisdom, precedent (protobufs??) or convention (naming or
otherwise) for how this should look?

[Bryan C. Mills]

My 2¢:

The interface type M should be a marker interface, with an unexported field and a reference implementation.

```

type Metadata struct {

isMetadata()

}

type EmptyMetadata struct {}

func (EmptyMetadata) isMetadata() {}

```

Then you can define the `Metadata` interface as, say, the union of the methods of the implementations (with an `ok` return-value indicating whether that particular property is available on the implementation).

That provides users with a place to look for documentation, plus niceties like type-based IDE autocompletion — but the unexported method forces implementors to embed some other implementation (such as `EmptyMetadata`), which allows you to expand the interface in the future without a breaking change.

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAOeFMNU8DW454vJyZPitUnkC2tTb5g8ft4P0jpn5fJU317S_fg%40mail.gmail.com.

[Nigel Tao]

On Fri, Aug 16, 2019 at 8:10 AM 'Bryan C. Mills' via golang-dev
<golan...@googlegroups.com> wrote:
> type Metadata struct {

In case anyone else was momentarily confused, "struct" is a typo, and
should be "interface".

> Then you can define the `Metadata` interface as, say, the union of the methods of the implementations

That does have its benefits, but IIUC, as I said, that'll make "image"
transitively depend on "encoding/xml". Also, "union... of the
implementations" is a tricky concept. One of the purposes of the image
package is that third parties can define their own codecs, which could
have their own metadata.

[Sam Whited]

On Thu, Aug 15, 2019, at 22:35, Nigel Tao wrote:
> That does have its benefits, but IIUC, as I said, that'll make "image"
> transitively depend on "encoding/xml". Also, "union... of the
> implementations" is a tricky concept. One of the purposes of the image
> package is that third parties can define their own codecs, which could
> have their own metadata.

Does this mean that metadata is always decoded immediately when decoding
the image? I'd probably prefer to decode it later (if at all), or first
(and then only decode the image under certain circumstances but not have
to decode the metadata all over again) if possible. I wonder if it would
be better to just keep metadata as raw bytes (or a reader so that
implementations could decide whether to buffer or point directly into
the file for as long as it was still open) and maybe a marker type for
what type of metadata was discovered.

Then if you get eg. PNG metadata out that can have its own field for XMP
or EXIF metadata bytes and the PNG package can have functionality for
decoding the PNG metadata into a struct and the metadata/xmp package can
have structs for use with the encoding/xml package.

Eg.

type RGBA struct {
…
Metadata []byte
Type MetadataType
}

// This could just be a string constant that varies by package,
// or it could be a key that you you type switch on similar to
// context keys so that we know there are no conflicts, or we could
// just leave it out entirely and assume that if you decode an image
// with the PNG package you have to remember what type its metadata
// is likely to be.
type MetadataType interface{}

switch myImage.Type.(type) {
case xmp.MetadataKey:
xmpMetadata, err := xmp.Decode(myImage.Metadata)
…
case png.MetadataKey:
…
}


This also means we don't have to use the "registration" pattern for
metadata which I personally don't like (and means we don't have to
iterate over all registered metadata types if the user already knows
what type its going to be).

—Sam


--
Sam Whited

[Nigel Tao]

On Fri, Aug 16, 2019 at 8:58 AM Sam Whited <s...@samwhited.com> wrote:
> type RGBA struct {
> …
> Metadata []byte
> Type MetadataType
> }

Yeah, []byte values (lazy decoding) does have its benefits.

One thing is that an encoded image can have multiple types of
metadata: instead of a PNG metadata type, perhaps a better model is
that PNGs can contain EXIF metadata, ICCP metadata, etc.

In other words, the new field(s) could be something like

type RGBA struct {
...
Metadata map[MetadataType][]byte

}


> This also means we don't have to use the "registration" pattern for
> metadata which I personally don't like (and means we don't have to
> iterate over all registered metadata types if the user already knows
> what type its going to be).

Yeah, in hindsight, the image.RegisterFormat mechanism - global state
conventionally configured by underscore imports - was a mistake.
Instead, having image.Decode take an explicit list of supported
formats would have been better.

[Robert Engels]

I think the registration part is required. In most cases image related programs are format agnostic or try to be.

> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAOeFMNXjhwrwRBOYGcZpWLkHhthmCuYwMdK_v6szbCKGPGoZAA%40mail.gmail.com.

[Nigel Tao]

On Fri., 16 Aug. 2019, 10:52 Robert Engels, <ren...@ix.netcom.com> wrote:

I think the registration part is required. In most cases image related programs are format agnostic or try to be.

Sure, image related programs should work with a variety of formats. I'm saying that it would be better to have package main explicitly pass an []image.Format around, containing the intended list of supported formats, instead of relying on underscore import's having the silent init time side effect of modifying a global registry.

For example, package main cannot currently blacklist formats. Any one of its transitive dependencies may import something that does an init time registration.

[Robert Engels]

I disagree and most image frameworks I’ve worked with works this way for the simple reason that as new image formats are supported in the runtime you don’t need to make any code changes to handle them. The static compilation of Go makes this harder, but if anything, the codecs could be shipped as plugins.

This is of major concern for security updates - which is something I’ve pointed to as a problem with Go in other areas. It has a very large stdlib but doesn’t dynamically load it so it becomes a security risk - too much effort to update code in the wild - everything needs to be recompiled and redistributed. 

[Bryan C. Mills]

If there really isn't anything universal among the various Metadata formats — if the user needs to know the concrete implementation in order to know what the metadata contains — then why add a Metadata interface to the core `image` package at all?

In that case, I think you're on the right track in https://github.com/golang/go/issues/33457#issuecomment-521839711. I left a comment there with a bit more detail.

--

You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/BBC54244-5E7E-4CB9-A7CF-1E3811F592C0%40ix.netcom.com.

[Robert Engels]

I thing there is a lot of overlap in the metadata. Items like time, geo location, dpi, etc. are pretty standard. 

You can look at ImageIO in Java for a decent reference example for base metadata. 

Also, having a core method to get the metadata as a byte[] and write from a byte[] is very useful when performing image edits. Often you don’t need all of the custom meta data but you’d like to preserve it across edits. 

[Ian Lance Taylor]

On Fri, Aug 16, 2019 at 6:09 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> This is of major concern for security updates - which is something I’ve pointed to as a problem with Go in other areas. It has a very large stdlib but doesn’t dynamically load it so it becomes a security risk - too much effort to update code in the wild - everything needs to be recompiled and redistributed.

As a side note, while using a single shared library is often claimed
as a security advantage, it is just as easily seen as a security
disadvantage: when everything uses the same shared library, then with
a single change you can introduce a security hole into many programs.
There are advantages and disadvantages both ways.

Ian

[Robert Engels]

True, but that doesn’t seem to be the case in practice. That is, you really see new security holes caused by security fixes as the code is heavily reviewed. New major versions of the stdlib are another story - but nothing says the modular stdlib wouldn’t be by major version, similar to other envs.

[Than McIntosh]

Ian wrote:
> As a side note, while using a single shared library is often claimed
> as a security advantage, it is just as easily seen as a security
> disadvantage: when everything uses the same shared library, then with
> a single change you can introduce a security hole into many programs.

Robert wrote:
> True, but that doesn’t seem to be the case in practice.

Seems to me the library MSCTF.DLL would be a good example of this happening in practice (details in https://googleprojectzero.blogspot.com/2019/08/down-rabbit-hole.html).

Than

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/9D7226EB-944E-41B5-82D3-6FEAD1899273%40ix.netcom.com.

[Robert Engels]

You made my point. That DLL is for text services and it can certainly be updated independently of the apps that use it in order to make security fixes. You don’t need to die load a new OS or a new version of everything application that uses it. 

[Than McIntosh]

I think I am also making Ian's point as well. Create a single attack for the DLL, and now every Windows application that uses the DLL can be attacked.

[Robert Engels]

But if there is a security hole in the stdlib it can always be attacked whether in shared code or static. At least with shared code you have an easier way to correct all of them. 

[ptei...@fastly.com]

On Thursday, August 15, 2019 at 5:19:53 PM UTC-7, Nigel Tao wrote:

On Fri, Aug 16, 2019 at 8:58 AM Sam Whited <s...@samwhited.com> wrote:
>     type RGBA struct {
>         …
>         Metadata []byte
>         Type MetadataType
>     }

Yeah, []byte values (lazy decoding) does have its benefits.

Apologies for dropping into this discussion after a few weeks of inactivity, but I have an experience report that might be helpful. We've solved these things already, but would have used a built in metadata system if one had existed and would certainly switch if it makes sense.

Background: I'm working on an imaging product where we have augmented the image decoders to provide a few extra things in their Config structs. Things like:

• Embedded ICC color profiles
• EXIF orientation (though a full EXIF block would work just as well)
• JPEG progressive flag
• JPEG colorspace (gray/rgb/ycbcr/cmyk/ycck)
• JPEG quantization tables
• WebP lossless/lossy flag

Notably some of these things are more clearly image metadata (ICC, EXIF) and some more clearly format specific. A generic metadata API might be useful for the former, but not the latter.

[]byte API

A lazy ([]byte) API is really important for our usage. But just as important, we need to be able to get that data (particularly EXIF orientation and ICC color profiles) before any pixel data is decoded. So in my mind, the DecodeConfig API would e.g. reconstruct an ICC color profile from the JPEG markers that contain it but allow the application to make decoding decisions before getting into the pixels.

 

One thing is that an encoded image can have multiple types of
metadata: instead of a PNG metadata type, perhaps a better model is
that PNGs can contain EXIF metadata, ICCP metadata, etc.

Yes, and what's possible in the container files may not match what's seen in practice. Does the metadata API need to provide distinct access to all of the tEXt chunks in a png file? If so, suddenly this becomes a map from metadata type to many values.

The distinction I want to make on the "types of metadata" front is that there are definitely things which are generic metadata (ICC, EXIF, XMP) and things which are not (quantization tables, progressive flags, colorspace). It should be clear where more of either might go in the future, and maybe that's DecodeConfig vs Metadata but for our needs we've been able to put everything we need into DecodeConfig.

 

In other words, the new field(s) could be something like

type RGBA struct  {
  ...
  Metadata map[MetadataType][]byte
}

There's already a little tension in a map to []byte with multiple blocks of the same MetadataType in a file, and maybe that doesn't deserve to be solved in the standard library but at least the meaning of this map should be clear for those situations.

A motivating example: there are three different text metadata blocks for png, each of which can occur several times in the file. Would the metadata map normalize them all to MetadataType=Text or keep them separate? Does this decoder API normalize them all to strings? Does it concatenate together same blocks so a []byte can be returned?

It has been our experience that it feels cleaner to avoid the Image interface for these things entirely and to place the metadata on the format specific Config structs in ways that more closely match the underlying file. This allows e.g. png to include many text chunks, plus an accessor that makes using them more convenient (maybe a func() []string).

I understand the complications involved in expanding the standard library APIs and so I'm not suggesting that this is what we need in image/*, but hopefully these are good examples and I'd be very happy to join a deeper discussion.

Peter