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.
Created attachment 327749 [details] Patch
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
Btw, is there any test backing this implementation?
(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; }
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
Created attachment 331346 [details] Patch
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.
(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
Created attachment 331378 [details] Patch
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.
Created attachment 331478 [details] Patch
Comment on attachment 331478 [details] Patch Clearing flags on attachment: 331478 Committed r227067: <https://trac.webkit.org/changeset/227067>
All reviewed patches have been landed. Closing bug.
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.
(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