Repository: SerenityOS/serenity
File: Userland/Libraries/LibGfx/ImageFormats/JPEGLoader.cpp (C++)

Code before:
{
    if (m_context->exif_metadata)
        return *m_context->exif_metadata;
    return OptionalNone {};
}

ErrorOr<Optional<ReadonlyBytes>> JPEGImageDecoderPlugin::icc_data()
{
    if (m_context->icc_data.has_value())
        return *m_context->icc_data;
    return OptionalNone {};
}

Optional<Gps> JPEGImageDecoderPlugin::gps()
{
    if (m_context->exif_metadata && m_context->exif_metadata->gps_info().has_value()) {
        auto gps_longitude = m_context->exif_metadata->gps_longitude();
        double longitude = gps_longitude->at(0).as_double() + (gps_longitude->at(1).as_double() / 60.0) + (gps_longitude->at(2).as_double() / 3600.0);

        auto gps_latitude = m_context->exif_metadata->gps_latitude();
        double latitude = gps_latitude->at(0).as_double() + (gps_latitude->at(1).as_double() / 60.0) + (gps_latitude->at(2).as_double() / 3600.0);

        return Gps { latitude, longitude };
    }
    return OptionalNone {};
}

NaturalFrameFormat JPEGImageDecoderPlugin::natural_frame_format() const
{
    if (m_context->state == JPEGLoadingContext::State::Error)
        return NaturalFrameFormat::RGB;
    VERIFY(m_context->state >= JPEGLoadingContext::State::HeaderDecoded);
    if (m_context->components.size() == 1)
        return NaturalFrameFormat::Grayscale;
    if (m_context->components.size() == 4)
        return NaturalFrameFormat::CMYK;
    return NaturalFrameFormat::RGB;
}

ErrorOr<NonnullRefPtr<CMYKBitmap>> JPEGImageDecoderPlugin::cmyk_frame()
{
    VERIFY(natural_frame_format() == NaturalFrameFormat::CMYK);

    if (m_context->state < JPEGLoadingContext::State::BitmapDecoded) {
        if (auto result = decode_jpeg(*m_context); result.is_error()) {
            m_context->state = JPEGLoadingContext::State::Error;
            return result.release_error();
        }
        m_context->state = JPEGLoadingContext::State::BitmapDecoded;
    }


Review comment (refactor):
We definitely need to find a way to factorize this code, we can't duplicate that in every image decoder. Also, operations on GPS coordinates doesn't seem to be a job for an image decoder.

The `Metadata` type was designed to be extended with more complex subtypes (e.g. `ExifMetadata`), but these subtypes can still be accessed by the same API.

So in `PropertiesWindow.cpp`, you can have something like:
```cpp
if (auto const& metadata = image_decoder->metadata(); metadata.has_value() && is<ExifMetadata>(*metadata)) {
	auto gps = gps_from_exif(metadata);
	...
}
```

---

Now on this code snippet specifically:

This is user provided data, you shouldn't assume that having `gps_info` means that all flags are present. You should still check both `metadata.gps_longitude()` and `metadata.gps_latitude()`.

Also, please use the `operator[]` instead of `at()`.

Code after:
{
    if (m_context->exif_metadata)
        return *m_context->exif_metadata;
    return OptionalNone {};
}

ErrorOr<Optional<ReadonlyBytes>> JPEGImageDecoderPlugin::icc_data()
{
    if (m_context->icc_data.has_value())
        return *m_context->icc_data;
    return OptionalNone {};
}

NaturalFrameFormat JPEGImageDecoderPlugin::natural_frame_format() const
{
    if (m_context->state == JPEGLoadingContext::State::Error)
        return NaturalFrameFormat::RGB;
    VERIFY(m_context->state >= JPEGLoadingContext::State::HeaderDecoded);
    if (m_context->components.size() == 1)
        return NaturalFrameFormat::Grayscale;
    if (m_context->components.size() == 4)
        return NaturalFrameFormat::CMYK;
    return NaturalFrameFormat::RGB;
}

ErrorOr<NonnullRefPtr<CMYKBitmap>> JPEGImageDecoderPlugin::cmyk_frame()
{
    VERIFY(natural_frame_format() == NaturalFrameFormat::CMYK);

    if (m_context->state < JPEGLoadingContext::State::BitmapDecoded) {
        if (auto result = decode_jpeg(*m_context); result.is_error()) {
            m_context->state = JPEGLoadingContext::State::Error;
            return result.release_error();
        }
        m_context->state = JPEGLoadingContext::State::BitmapDecoded;
    }

    return *m_context->cmyk_bitmap;
}

}