Bug 180080 – [EME][GStreamer] Add the full-sample encryption support in the GStreamer ClearKey decryptor

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

[Yacine Bandou]

Currently the GStreamer ClearKey decryptor doesn't support the full-sample encryption, where the buffer is entirely encrypted,
it supports only the sub-sample encryption.

The sub-sample encryption: The buffer is divided into one or more subsamples. Each subsample have a clear part followed by an encrypted part.

The full-sample encryption: The entire buffer is encrypted.

[Yacine Bandou]

Created attachment 327749 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 327749 [details]
Patch

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

I think given the complexity of this algorithm and considering that we are inside a GStreamer element we can do some GStreamer-like coding. In this case, I think GStreamer gotos are a good option. In the subsamples section we have "park" that is going to free the reader and the subsamples map, it is going to fallback to beach after the if clause finishes and if is going to free the buffer map. In the whole buffer decryption we just fallback to beach.

This is not an r+ because it is not ready IMHO but it is not an r- either because technically I see nothing wrong.

What would be really nice would be to have something like UniquePtr but for this kind of stack variables that need some action to free them. Anyway I think it would be out of the scope of this bug unless you feel like doing it.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:238
> +    if (subSampleCount) {

Before this:

bool returnValue = true;

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:242
> +        gboolean subsamplesBufferMapped = gst_buffer_map(subSamplesBuffer, &subSamplesMap, GST_MAP_READ);

gboolean -> bool

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:246
> +            gst_buffer_unmap(buffer, &map);
> +            return false;

returnValue = false;
goto beach;

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:251
> +        unsigned sampleIndex = 0;

Move sampleIndex to below GST_DEBUG_OBJECT.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:257
> +            guint16 nBytesClear = 0;
> +            guint32 nBytesEncrypted = 0;

uint16_t and uint32_t

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:266
> +                    gst_byte_reader_free(reader);
> +                    gst_buffer_unmap(buffer, &map);
> +                    gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> +                    return false;

returnValue = false;
goto park;

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:285
> +                    gst_byte_reader_free(reader);
> +                    gst_buffer_unmap(buffer, &map);
> +                    gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> +                    return false;

ditto

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:291
> +        gst_byte_reader_free(reader);
> +        gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);

"park:" here

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:296
> +        guint32 nBytesEncrypted = map.size;

You don't need this variable

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:305
>                  gst_buffer_unmap(buffer, &map);
> -                gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
>                  return false;

returnValue = false;
goto beach;

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:309
>      gst_buffer_unmap(buffer, &map);

beach: here

[Xabier Rodríguez Calvar]

Btw, is there any test backing this implementation?

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 327749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327749&action=review
> 
> I think given the complexity of this algorithm and considering that we are
> inside a GStreamer element we can do some GStreamer-like coding. In this
> case, I think GStreamer gotos are a good option. In the subsamples section
> we have "park" that is going to free the reader and the subsamples map, it
> is going to fallback to beach after the if clause finishes and if is going
> to free the buffer map. In the whole buffer decryption we just fallback to
> beach.
> 
> This is not an r+ because it is not ready IMHO but it is not an r- either
> because technically I see nothing wrong.
> 
> What would be really nice would be to have something like UniquePtr but for
> this kind of stack variables that need some action to free them. Anyway I
> think it would be out of the scope of this bug unless you feel like doing it.
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:238
> > +    if (subSampleCount) {
> 
> Before this:
> 
> bool returnValue = true;
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:242
> > +        gboolean subsamplesBufferMapped = gst_buffer_map(subSamplesBuffer, &subSamplesMap, GST_MAP_READ);
> 
> gboolean -> bool
> 
It's not my code, it was already here

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:246
> > +            gst_buffer_unmap(buffer, &map);
> > +            return false;
> 
> returnValue = false;
> goto beach;
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:251
> > +        unsigned sampleIndex = 0;
> 
> Move sampleIndex to below GST_DEBUG_OBJECT.
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:257
> > +            guint16 nBytesClear = 0;
> > +            guint32 nBytesEncrypted = 0;
> 
> uint16_t and uint32_t
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:266
> > +                    gst_byte_reader_free(reader);
> > +                    gst_buffer_unmap(buffer, &map);
> > +                    gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> > +                    return false;
> 
> returnValue = false;
> goto park;
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:285
> > +                    gst_byte_reader_free(reader);
> > +                    gst_buffer_unmap(buffer, &map);
> > +                    gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> > +                    return false;
> 
> ditto
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:291
> > +        gst_byte_reader_free(reader);
> > +        gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> 
> "park:" here
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:296
> > +        guint32 nBytesEncrypted = map.size;
> 
> You don't need this variable
> 

Ok I'll remove it and I'll use map.size instead

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:305
> >                  gst_buffer_unmap(buffer, &map);
> > -                gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
> >                  return false;
> 
> returnValue = false;
> goto beach;
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:309
> >      gst_buffer_unmap(buffer, &map);
> 
> beach: here


I think there is a misunderstanding during the patch review

Here is the diff by ignoring the space change:

--- a/Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp
+++ b/Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp
@@ -235,6 +235,8 @@ static gboolean webKitMediaClearKeyDecryptorDecrypt(WebKitMediaCommonEncryptionD
         GST_ERROR_OBJECT(self, "Failed to map buffer");
         return false;
     }
+    if (subSampleCount) {
+        // Subsample encryption.
 
         GstMapInfo subSamplesMap;
         gboolean subsamplesBufferMapped = gst_buffer_map(subSamplesBuffer, &subSamplesMap, GST_MAP_READ);
@@ -276,7 +278,7 @@ static gboolean webKitMediaClearKeyDecryptorDecrypt(WebKitMediaCommonEncryptionD
                 GST_TRACE_OBJECT(self, "%d bytes encrypted (todo=%zu)", nBytesEncrypted, map.size - position);
                 error = gcry_cipher_decrypt(priv->handle, map.data + position, nBytesEncrypted, 0, 0);
                 if (error) {
-                GST_ERROR_OBJECT(self, "decryption failed: %s", gpg_strerror(error));
+                    GST_ERROR_OBJECT(self, "sub sample decryption failed: %s", gpg_strerror(error));
                     gst_byte_reader_free(reader);
                     gst_buffer_unmap(buffer, &map);
                     gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
@@ -285,10 +287,27 @@ static gboolean webKitMediaClearKeyDecryptorDecrypt(WebKitMediaCommonEncryptionD
                 position += nBytesEncrypted;
             }
         }
-
         gst_byte_reader_free(reader);
-    gst_buffer_unmap(buffer, &map);
         gst_buffer_unmap(subSamplesBuffer, &subSamplesMap);
+
+    } else {
+        // Full sample encryption.
+
+        guint32 nBytesEncrypted = map.size;
+        GST_TRACE_OBJECT(self, "full sample encryption: %d encrypted bytes", nBytesEncrypted);
+
+        // Check if the buffer is empty.
+        if (nBytesEncrypted) {
+            error = gcry_cipher_decrypt(priv->handle, map.data, nBytesEncrypted, 0, 0);
+            if (error) {
+                GST_ERROR_OBJECT(self, "full sample decryption failed: %s", gpg_strerror(error));
+                gst_buffer_unmap(buffer, &map);
+                return false;
+            }
+        }
+    }
+    gst_buffer_unmap(buffer, &map);
+
     return true;
 }

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #3)
> Btw, is there any test backing this implementation?

These patches: 180071, 180081, 180083 with this one, allow us pass this EME ClearKey test https://cpearce.github.io/mse-eme/

The content media of this test is in full-sample encryption mode.

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #4)
> > gboolean -> bool
> > 
> It's not my code, it was already here

I happens a lot of times to me too. I'm not in favor of changing things when they are wrong if it's just a style issue just for the style itself but if I have to change the line, I prefer to correct the style too.

> I think there is a misunderstanding during the patch review
> 
> Here is the diff by ignoring the space change:

Oh, I see. But you're modifying those lines anyway and they will be reflected like your change in the diff and as I say before, better to correct the things that we can when we touch those lines.

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #5)
> (In reply to Xabier Rodríguez Calvar from comment #3)
> > Btw, is there any test backing this implementation?
> 
> These patches: 180071, 180081, 180083 with this one, allow us pass this EME
> ClearKey test https://cpearce.github.io/mse-eme/
> 
> The content media of this test is in full-sample encryption mode.

Seems good. I'd suggest you import the test into WebKit then, cause adding all this code without a test to back it up is a bad idea. I don't know if it's possible to use the test importer (I doubt it). But I'd import the test into WebKit and acknowledge the author/s in that commit.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #7)
> > The content media of this test is in full-sample encryption mode.
> 
> Seems good. I'd suggest you import the test into WebKit then, cause adding
> all this code without a test to back it up is a bad idea. I don't know if
> it's possible to use the test importer (I doubt it). But I'd import the test
> into WebKit and acknowledge the author/s in that commit.

Actually, maybe the idea would be to add the test before, flag it as failure because we lack the proper code, add dependencies to the bugs that implement the fix for it and when everything is ok, unflag it.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #6)
> (In reply to Yacine Bandou from comment #4)
> > > gboolean -> bool
> > > 
> > It's not my code, it was already here
> 
> I happens a lot of times to me too. I'm not in favor of changing things when
> they are wrong if it's just a style issue just for the style itself but if I
> have to change the line, I prefer to correct the style too.
> 
> > I think there is a misunderstanding during the patch review
> > 
> > Here is the diff by ignoring the space change:
> 
> Oh, I see. But you're modifying those lines anyway and they will be
> reflected like your change in the diff and as I say before, better to
> correct the things that we can when we touch those lines.

understood and I agree.

But it depends on the reviewer, there are who do not like to change the existing code just for the style.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #8)
> (In reply to Xabier Rodríguez Calvar from comment #7)
> > > The content media of this test is in full-sample encryption mode.
> > 
> > Seems good. I'd suggest you import the test into WebKit then, cause adding
> > all this code without a test to back it up is a bad idea. I don't know if
> > it's possible to use the test importer (I doubt it). But I'd import the test
> > into WebKit and acknowledge the author/s in that commit.
> 
> Actually, maybe the idea would be to add the test before, flag it as failure
> because we lack the proper code, add dependencies to the bugs that implement
> the fix for it and when everything is ok, unflag it.

understood, I'll see how can do it.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 327749 [details]
> Patch
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:238
> > +    if (subSampleCount) {
> 
> Before this:
> 
> bool returnValue = true;

I agree.

> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:242
> > +        gboolean subsamplesBufferMapped = gst_buffer_map(subSamplesBuffer, &subSamplesMap, GST_MAP_READ);
> 
> gboolean -> bool

It is a GStreamer element, long term, I think we should extract the GstDecryptor from WebKit and put it in gst-plugin-bad for example.

For that, we should keep the GStreamer types and style instead of WebKit types or style.

> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:246
> > +            gst_buffer_unmap(buffer, &map);
> > +            return false;
> 
> returnValue = false;
> goto beach;
> 

I agree.

> > SSource/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:296
> > +        guint32 nBytesEncrypted = map.size;
> 
> You don't need this variable
> 

I agree.

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #11)
> It is a GStreamer element, long term, I think we should extract the
> GstDecryptor from WebKit and put it in gst-plugin-bad for example.
> 
> For that, we should keep the GStreamer types and style instead of WebKit
> types or style.

I agree that we could do that in the long term. Nowadays, it is inside WebKit codebase and follows (or should) its coding style.

[Yacine Bandou]

Created attachment 331346 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 331346 [details]
Patch

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

Shouldn't this patch also remove avoid the crash flagged in r226966

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:242
> +    GstByteReader* reader;
> +    unsigned position = 0;
> +    unsigned sampleIndex = 0;

Can we leave this variables where they're used or is the compiler complaining?

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:244
> +    bool subsamplesBufferMapped;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:252
> +            error = gcry_cipher_decrypt(priv->handle, map.data, map.size, 0, 0);

We should rename this variable to something more meaningful like cypherError.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:289
> +        GST_TRACE_OBJECT(self, "subsample index %d - %d bytes clear (todo=%zu)", sampleIndex, nBytesClear, map.size - position);

If I am not mistaken, sampleIndex should be printed with %u and nBytesClear with %hu.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:292
> +            GST_TRACE_OBJECT(self, "subsample index %d - %d bytes encrypted (todo=%zu)", sampleIndex, nBytesEncrypted, map.size - position);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:295
> +                GST_ERROR_OBJECT(self, "sub sample index %d decryption failed: %s", sampleIndex, gpg_strerror(error));

Ditto.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:302
> +freeReader:

I think we should rename this as releaseSubsamples

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:306
> +unmapBuffer:

I think we should rename this as releaseBuffer.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #14)
> Comment on attachment 331346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331346&action=review
> 
> Shouldn't this patch also remove avoid the crash flagged in r226966
> 

Yes, exactly
I'll add explicitly check of the subSampleBuffer.
  

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:242
> > +    GstByteReader* reader;
> > +    unsigned position = 0;
> > +    unsigned sampleIndex = 0;
> 
> Can we leave this variables where they're used or is the compiler
> complaining?
>
for : 
GstByteReader* reader; 
unsigned position = 0;

The compiler complaining, I get a "cross initialization" 

for:
unsigned sampleIndex = 0; 

I'll move it.

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:244
> > +    bool subsamplesBufferMapped;
> 
> Ditto.
> 

I'll remove it.

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:252
> > +            error = gcry_cipher_decrypt(priv->handle, map.data, map.size, 0, 0);
> 
> We should rename this variable to something more meaningful like cypherError.
> 

I agree.


> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:289
> > +        GST_TRACE_OBJECT(self, "subsample index %d - %d bytes clear (todo=%zu)", sampleIndex, nBytesClear, map.size - position);
> 
> If I am not mistaken, sampleIndex should be printed with %u and nBytesClear
> with %hu.
> 

Yes

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:292
> > +            GST_TRACE_OBJECT(self, "subsample index %d - %d bytes encrypted (todo=%zu)", sampleIndex, nBytesEncrypted, map.size - position);
> 
> Ditto.
> 
Yes 

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:295
> > +                GST_ERROR_OBJECT(self, "sub sample index %d decryption failed: %s", sampleIndex, gpg_strerror(error));
> 
> Ditto.
>
 
Yes

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:302
> > +freeReader:
> 
> I think we should rename this as releaseSubsamples
> 

OK

> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:306
> > +unmapBuffer:
> 
> I think we should rename this as releaseBuffer.

OK

[Yacine Bandou]

Created attachment 331378 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 331378 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:210
> +    // Check ivBuffer isn't null

Period at the end.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:238
> +    // Check buffer isn't null

Period at the end.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:271
> +    // Check subSamplesBuffer isn't null

Period at the end.

[Yacine Bandou]

Created attachment 331478 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331478 [details]
Patch

Clearing flags on attachment: 331478

Committed r227067: <https://trac.webkit.org/changeset/227067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Comment on attachment 331478 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:293
> +        unsigned sampleIndex = 0;
>  
>          if (sampleIndex < subSampleCount) {

This is wrong. Now sampleIndex is unconditionally 0. The assignment should be moved outside of the loop.

[Yacine Bandou]

(In reply to Philippe Normand from comment #21)
> Comment on attachment 331478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331478&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:293
> > +        unsigned sampleIndex = 0;
> >  
> >          if (sampleIndex < subSampleCount) {
> 
> This is wrong. Now sampleIndex is unconditionally 0. The assignment should
> be moved outside of the loop.

I agree, I created a bug to fix it https://bugs.webkit.org/show_bug.cgi?id=185382