[Webkit-unassigned] [Bug 181990] [EME][GStreamer] Add support for the encrypted Caps in GStreamerUtilities

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=181990

Xabier Rodríguez Calvar <calvaris at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #332984|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #11 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 332984
  --> https://bugs.webkit.org/attachment.cgi?id=332984
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180208/abce520d/attachment.html>