Bug 180370 - [GStreamer] <img> tag needs to support video formats
Summary: [GStreamer] <img> tag needs to support video formats
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks: 211995
  Show dependency treegraph
 
Reported: 2017-12-04 12:48 PST by Michael Catanzaro
Modified: 2020-06-14 04:33 PDT (History)
18 users (show)

See Also:


Attachments
Patch (32.77 KB, patch)
2020-05-17 05:27 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2020-05-17 09:58 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (32.91 KB, patch)
2020-05-20 02:10 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-12-04 12:48:44 PST
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.
Comment 1 Miguel Gomez 2018-05-31 03:14:59 PDT
Adding fast/images/animated-image-mp4-crash.html here as well, which is timing out.
Comment 2 Philippe Normand 2020-05-09 10:51:50 PDT
I... kinda... started having a look... :D
Comment 3 Philippe Normand 2020-05-17 05:27:34 PDT
Created attachment 399588 [details]
Patch
Comment 4 Philippe Normand 2020-05-17 09:58:29 PDT
Created attachment 399594 [details]
Patch
Comment 5 Xabier Rodríguez Calvar 2020-05-18 07:12:48 PDT
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?
Comment 6 Philippe Normand 2020-05-20 02:10:20 PDT
Created attachment 399822 [details]
Patch
Comment 7 Philippe Normand 2020-05-20 02:11:50 PDT
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 :)
Comment 8 Xabier Rodríguez Calvar 2020-05-20 07:28:21 PDT
Comment on attachment 399822 [details]
Patch

I still see lots of SharedBuffer& not turned into const, please do that.
Comment 9 Philippe Normand 2020-05-20 07:34:42 PDT
Those are part of the ImageDecoder API. Why should I change all the implementations for the sake of another one?
Comment 10 Xabier Rodríguez Calvar 2020-05-20 08:15:16 PDT
(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.
Comment 11 EWS 2020-05-20 08:52:40 PDT
Committed r261922: <https://trac.webkit.org/changeset/261922>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399822 [details].
Comment 12 Philippe Normand 2020-06-14 04:33:39 PDT
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