Bug 181990 – [EME][GStreamer] Add support for the encrypted Caps in GStreamerUtilities

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Yacine Bandou]

Add the support of encrypted Caps in GStreamer utilities and refactoring the manner that the Caps if managed,
like how extract the resolution in the video Caps or how check if the Caps is encrypted.

[Yacine Bandou]

Created attachment 332151 [details]
Patch

[Yacine Bandou]

Created attachment 332152 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 332152 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:63
> +    if (!isEncryptedCaps(caps)) {

Let's exchange if and else

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:81
> +        gint w = 0, h = 0;
> +        gint parN = 1, parD = 1;

These need to become int and get meaningful names like width, height, pixelAspectRatioNumerator, pixelAspectRatioDenominator.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:137
> +

Unneeded extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:139
> +        GST_ERROR("caps is null !!!");

is -> are and remove the !!!

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:141
> +    if (!caps) {
> +        GST_ERROR("caps is null !!!");
> +        return nullptr;
> +    }

If I am not mistaken, this is not common at all and I don't think we need to check it and error. Let's ASSERT instead if you didn't find any example that makes this crash consistently.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:145
> +        GST_ERROR("caps is empty !!!");

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:147
> +    if (!structure) {
> +        GST_ERROR("caps is empty !!!");
> +        return nullptr;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:157
> +    const gchar* mediaType = getMediaTypeFromCaps(caps);

gchar -> char

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
> +    if (!mediaType) {
> +        GST_ERROR("Failed to get MediaType !!!");
> +        return false;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> +    return g_str_has_prefix(mediaType, "video");

we can keep the check for video/

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:167
> +    const gchar* mediaType = getMediaTypeFromCaps(caps);

gchar -> char

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:171
> +    if (!mediaType) {
> +        GST_ERROR("Failed to get MediaType !!!");
> +        return false;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:172
> +    return g_str_has_prefix(mediaType, "audio");

we can keep the check of audio/

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:177
> +    const gchar* mediaType = getMediaTypeFromCaps(caps);

char

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:181
> +    if (!mediaType) {
> +        GST_ERROR("Failed to get MediaType !!!");
> +        return false;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:182
> +    return g_str_has_prefix(mediaType, "text");

we can keep the check for text/

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:191
> +    if (!caps) {
> +        GST_ERROR("caps is null !!!");
> +        return false;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:196
> +    if (!structure) {
> +        GST_ERROR("caps is empty !!!");
> +        return false;
> +    }

ditto

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:65
> +bool isVideoCaps(GstCaps*);
> +bool isAudioCaps(GstCaps*);
> +bool isTextCaps(GstCaps*);
> +bool isEncryptedCaps(GstCaps*);

is -> are

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:598
> +    const gchar* originalMediaType = getMediaTypeFromCaps(m_demuxerSrcPadCaps.get());

char

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:606
> +        int parN, parD, stride;

names

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:36
> +    GstCaps* originCap = m_caps.get();

Make a GRefPtr and we can forget about unreffing later.

Name should be originalCaps if I understand the purpose correctly.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:39
> +

extra line

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:49
> +            const gchar* fieldName = gst_structure_nth_field_name(structure, j);

char

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:55
> +    if (isEncryptedCaps(originCap)) {
> +
> +        originCap =  gst_caps_copy(m_caps.get());
> +        GstStructure* structure = gst_caps_get_structure(originCap, 0);
> +
> +        if (!gst_structure_has_field(structure, "original-media-type"))
> +            return String();
> +
> +        gst_structure_set_name(structure, gst_structure_get_string(structure, "original-media-type"));
> +        // Remove the DRM related fields from the caps.
> +        for (int j = 0; j < gst_structure_n_fields(structure); ++j) {
> +            const gchar* fieldName = gst_structure_nth_field_name(structure, j);
> +
> +            if (g_str_has_prefix(fieldName, "protection-system")
> +                || g_str_has_prefix(fieldName, "original-media-type"))
> +                gst_structure_remove_field(structure, fieldName);
> +        }
> +    }

This operation is heavy. I think we can safely cache the codecName as a class attribute.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:746
> +        int parN, parD, stride;

names

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:180
> +    const gchar* mediaType = getMediaTypeFromCaps(caps);

char

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:185
> +    if (isEncryptedCaps(caps)) {
> +        GST_DEBUG_OBJECT(webKitMediaSrc, "PlaybackPipeline, with encrypted content the appropriate parser will be auto plugged by the decodebin after the decryptor in pipeline!");

This seems to belong to another patch, right?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:466
> +            width = size.width();
> +            height = size.height() * ((float) parD / (float) parN);
> +            stream->presentationSize = WebCore::FloatSize(width, height);

I've seen this operation quite a lot. I think we can create a helper function in utils to return the an IntSize. Of course in this case it will be converted to FloatSize.

[Yacine Bandou]

Created attachment 332388 [details]
Patch

[Charlie Turner]

Comment on attachment 332388 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:63
> +        if (!areVideoCaps(caps))

Can this check not be hoisted above the areEncryptedCaps? It's a valid pre-condition check for the whole function it seems. I know gst_video_info_from_caps does it's own validation in addition to this, but having it guarded by areEncryptedCaps doesn't immediately make sense to me.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:76
> +

blanks lines before and after else, there's more in the patch, please remove.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:98
> +    gint pixelAspectRatioNumerator, pixelAspectRatioDenominator;

I would assign these to 1 like in the original code, if the gst_structure_get_fraction call fails for some weird reason, we could get undefined behaviour in the height calculation.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:101
> +        if (!areVideoCaps(caps))

Ditto for pre-condition check

[Yacine Bandou]

Created attachment 332968 [details]
Patch

[Charlie Turner]

Comment on attachment 332968 [details]
Patch

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

Forgot to set the r- flag in previous review.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184
> +bool areVideoCaps(GstCaps* caps)

The are*Caps functions are all identical apart from the media type prefix string, could factor these all into one method such has capsIsType(GstCaps, AsciiLiteral)

[Yacine Bandou]

Created attachment 332984 [details]
Patch

[Charlie Turner]

Thanks Yacine, this LGTM, but I'm not an official reviewer. Will let calvaris have the final say :)

[Yacine Bandou]

(In reply to Charlie Turner from comment #9)
> Thanks Yacine, this LGTM, but I'm not an official reviewer. Will let
> calvaris have the final say :)

No problem Charlie, thanks for your review.

[Xabier Rodríguez Calvar]

Comment on attachment 332984 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Add the support of encrypted Caps in GStreamerUtilities.
> +        Refactor the manner that the Caps are handled, such as how to extract the resolution
> +        from the video Caps or how to check if the Caps are encrypted.

caps can be non-capital

> Source/WebCore/ChangeLog:24
> +

Extra line not needed.

> Source/WebCore/ChangeLog:28
> +        (WebCore::areEncryptedCaps): Add a new functions in order to handle the Caps properly.

caps

> Source/WebCore/ChangeLog:29
> +

Extra line not needed.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:63
> +        GST_ERROR("Failed to get the video size and foramt, they are not a video caps");

foramt -> format

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:68
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:70
> +        gint width = 0, height = 0;

gint -> int and variables need to be moved to just before where they are used.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:76
> +        gst_structure_get_fraction(structure, "pixel-aspect-ratio", &pixelAspectRatioNumerator, &pixelAspectRatioDenominator);

Assign these two variables to 1 before calling this.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:80
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:97
> +bool getVideoResolutionFromCaps(GstCaps* caps, float& width, float& height)

From what I see about this function it is only used to populate FloatSize attributes. Please turn it into std::optional<FloatSize> videoResolutionFromCaps(const GstCaps* caps)

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:100
> +    gint lWidth = 0, lHeight = 0;
> +    gint pixelAspectRatioNumerator = 1, pixelAspectRatioDenominator = 1;

Move this variables to where they are used.

gint -> int

l* -> gstreamer*

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:103
> +        GST_ERROR("Failed to get the video resolution, they are not a video caps");

ERROR -> WARNING

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:108
> +

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:115
>  

Unnecessary extra line

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:126
> +    }
> +    width = lWidth;

You love not needed extra lines and here, where you could use one, you don't :)

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:127
> +    height = lHeight * ((float) pixelAspectRatioNumerator / (float) pixelAspectRatioDenominator);

use c++ casts.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169
> +const char* getMediaTypeFromCaps(GstCaps* caps)

getMediaTypeFromCaps -> capsMediaType and make it tame const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176
> +    if (!structure) {
> +        GST_ERROR("caps are empty");
> +        return nullptr;
> +    }

Did you ever find any case where this error was hit? I doubt it, so I think it is better to just ASSERT on this. If this is something that really can happen sometimes, we should not just ERROR, it looks the kind of situation that should be managed by the caller instead, so we could WARNING here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184
> +bool capsIsType(GstCaps* caps, const char* type)

capsIsType -> doCapsHaveType and make it take const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:188
> +        GST_ERROR("Failed to get MediaType");

Contrary to the other two functions here, I don't think we should ASSERT here, but ERROR is too string, let's do WARNING.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:194
> +bool areEncryptedCaps(GstCaps* caps)

const caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:202
> +    if (!structure) {
> +        GST_ERROR("caps are empty");
> +        return false;
> +    }

Same as in capsMediaType.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:55
> +#define GST_VIDEO_CAPS_TYPE  "video/"
> +#define GST_AUDIO_CAPS_TYPE  "audio/"
> +#define GST_TEXT_CAPS_TYPE   "text/"

GST_*_CAPS_TYPE_PREFIX

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:57
> +    GRefPtr<GstCaps> originalCap = m_caps;

originalCaps

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:64
> +            return String();

you can return AtomicString() here.

> Source/WebCore/platform/graphics/gstreamer/mse/GStreamerMediaDescription.cpp:86
> +            codecName.remove(braceStart, braceEnd-braceStart);

braceEnd - braceStart

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #11)
> Comment on attachment 332984 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332984&action=review
> 

I agree with your review except these: 

> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:70
> > +        gint width = 0, height = 0;
> 
> gint -> int and variables need to be moved to just before where they are
> used.
>

why it needs to be moved ? it's a code style or it's an optimization?

> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:76
> > +        gst_structure_get_fraction(structure, "pixel-aspect-ratio", &pixelAspectRatioNumerator, &pixelAspectRatioDenominator);
> 
> Assign these two variables to 1 before calling this.

I prefer do it when "gst_structure_get_fraction" return false. 
 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169
> > +const char* getMediaTypeFromCaps(GstCaps* caps)
> 
> getMediaTypeFromCaps -> capsMediaType and make it tame const caps
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176
> > +    if (!structure) {
> > +        GST_ERROR("caps are empty");
> > +        return nullptr;
> > +    }
> 
> Did you ever find any case where this error was hit? I doubt it, so I think
> it is better to just ASSERT on this. If this is something that really can
> happen sometimes, we should not just ERROR, it looks the kind of situation
> that should be managed by the caller instead, so we could WARNING here.
> 

Yes, the caps can be empty.
I agree, I should use WARNING instead of ERROR.

[Yacine Bandou]

Created attachment 333381 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #12)
> > > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:70
> > > +        gint width = 0, height = 0;
> > 
> > gint -> int and variables need to be moved to just before where they are
> > used.
> >
> 
> why it needs to be moved ? it's a code style or it's an optimization?

It's coding style. We try to keep it like this as thoroughly as possible though it is true that in some cases, there's still code like this and even reviewers don't realized of this issues when reviewing. Anyway, all new code should respect the coding style and yes, this is part of it.

> > > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:76
> > > +        gst_structure_get_fraction(structure, "pixel-aspect-ratio", &pixelAspectRatioNumerator, &pixelAspectRatioDenominator);
> > 
> > Assign these two variables to 1 before calling this.
> 
> I prefer do it when "gst_structure_get_fraction" return false. 

Ok.

> > > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:169
> > > +const char* getMediaTypeFromCaps(GstCaps* caps)
> > 
> > getMediaTypeFromCaps -> capsMediaType and make it tame const caps
> > 
> > > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:176
> > > +    if (!structure) {
> > > +        GST_ERROR("caps are empty");
> > > +        return nullptr;
> > > +    }
> > 
> > Did you ever find any case where this error was hit? I doubt it, so I think
> > it is better to just ASSERT on this. If this is something that really can
> > happen sometimes, we should not just ERROR, it looks the kind of situation
> > that should be managed by the caller instead, so we could WARNING here.
> > 
> 
> Yes, the caps can be empty.
> I agree, I should use WARNING instead of ERROR.

Ok.

[Xabier Rodríguez Calvar]

Comment on attachment 333381 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:63
> +        GST_ERROR("Failed to get the video size and format, they are not a video caps");

ERROR -> WARNING. caps is always plural so "these are not video caps".

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:75
> +            pixelAspectRatioNumerator = pixelAspectRatioDenominator = 1;

This goes against coding style, one instruction at a line.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:99
> +        GST_WARNING("Failed to get the video resolution, they are not a video caps");

these are not video caps

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:100
> +        return std::optional<FloatSize>();

You need to return std::nullopt here, otherwise asking for has_value will answer always yes.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:123
> +    return std::optional<FloatSize>(FloatSize(width, height * (static_cast<float>(pixelAspectRatioNumerator) / static_cast<float>(pixelAspectRatioDenominator))));

You can use std::make_optional

[Yacine Bandou]

Created attachment 333474 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 333474 [details]
Patch

Clearing flags on attachment: 333474

Committed r228316: <https://trac.webkit.org/changeset/228316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.