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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 8 07:57:58 PST 2018


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

--- Comment #12 from Yacine Bandou <bandou.yacine at gmail.com> ---
(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.

-- 
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/0cd0a530/attachment.html>


More information about the webkit-unassigned mailing list