Bug 180370 – [GStreamer] <img> tag needs to support video formats

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

I'm adding a skip expectation for layout test http/tests/images/image-supports-video.html, which ensures that video formats can be used in an image tag. I think the expected behavior is for the first frame of the image to be displayed. Unfortunately websites are actually doing this.

I had previously skipped fast/images/animated-image-mp4.html for the same reason. I'll associate that expectation with this bug as well.

[Miguel Gomez]

Adding fast/images/animated-image-mp4-crash.html here as well, which is timing out.

[Philippe Normand]

I... kinda... started having a look... :D

[Philippe Normand]

Created attachment 399588 [details]
Patch

[Philippe Normand]

Created attachment 399594 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 399594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:65
> +        m_image = ImageGStreamer::createImage(gst_sample_ref(platformSample().sample.gstSample));

From what I am seeing, I think you're leaking the sample here. It is true that the ImageGStreamer::createImage adopts, but it adopts the ImageGStreamer object, not the sample (at least in the Cairo Impl).

This said, I think this API is confusing and it could be well using either a const GstSample* or maybe a GRefPtr<GstSample>&&. In this case the pointer would be destroyed after the call because nobody will use it.

> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:351
> +void ImageDecoderGStreamer::pushEncodedData(SharedBuffer& buffer)

Maybe you can switch to const SharedBuffer& all the way in the onion

> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:102
> +        ImageDecoderGStreamer* m_decoder;

Would it make sense to make this refcounted or weak?

[Philippe Normand]

Created attachment 399822 [details]
Patch

[Philippe Normand]

Comment on attachment 399594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:65
>> +        m_image = ImageGStreamer::createImage(gst_sample_ref(platformSample().sample.gstSample));
> 
> From what I am seeing, I think you're leaking the sample here. It is true that the ImageGStreamer::createImage adopts, but it adopts the ImageGStreamer object, not the sample (at least in the Cairo Impl).
> 
> This said, I think this API is confusing and it could be well using either a const GstSample* or maybe a GRefPtr<GstSample>&&. In this case the pointer would be destroyed after the call because nobody will use it.

Can't be const GstSample* because the gst_sample_* APIs expect a non-const pointer. I don't really see the point of using a GRefPtr either because the sample is already owned by the MediaSampleGStreamer :)

[Xabier Rodríguez Calvar]

Comment on attachment 399822 [details]
Patch

I still see lots of SharedBuffer& not turned into const, please do that.

[Philippe Normand]

Those are part of the ImageDecoder API. Why should I change all the implementations for the sake of another one?

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #9)
> Those are part of the ImageDecoder API. Why should I change all the
> implementations for the sake of another one?

ok, you might go ahead.

[EWS]

Committed r261922: <https://trac.webkit.org/changeset/261922>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399822 [details].

[Philippe Normand]

Comment on attachment 399594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:102
>> +        ImageDecoderGStreamer* m_decoder;
> 
> Would it make sense to make this refcounted or weak?

Making it weak wasn't a good idea: https://bugs.webkit.org/show_bug.cgi?id=213178