Relax h264/h265 codec check for MSE

[emutavchi]

Some platforms disable/remove software parsers because of the way encrypted playback is implemented. But, we still want working MSE playback there.

[eocanha]

The problem is that this patch might be breaking things on the rest of the platforms that aren't those "some platforms" you mention. I think some kind of ifdef similar to this one would be needed in order to remove the condition only for the platforms that don't require H264/H265 parsers.

[emutavchi]

Hi @eocanha. h264 parser is optional in AppendPipeline, h265 isn't used there. What's the reason for enforcing the parser check for MSE here?

Could you please elaborate on the platforms that might suffer from this change, and what things it could break?

[eocanha]

The parser at AppendPipeline level "might" not be needed (I removed it and the couple of MSE tests I checked still pass), but it's definitively needed in the playback pipeline. For instance, the OpenMAX decoder in the Raspberry Pi only supports format=byte-stream, and the MSE tests supply h264 in format=avc. Video playback wouldn't work without h264parse in that scenario.

Look at the supported caps:

  SINK template: 'sink'
    Availability: Always
    Capabilities:
      video/x-h264
              alignment: au
          stream-format: byte-stream
                  width: [ 1, 2147483647 ]
                 height: [ 1, 2147483647 ]

[emutavchi]

@eocanha but that applies to all h264 playback on RPI, not only MSE. Should RPI be handled separately in this case? i.e. completely disable h264/h265 if parsers are missing

[eocanha]

It's not only the RPI decoder. We have that case on upstream desktop too: avdec_h264 supports both avc and byte-stream out of the box, but openh264dec only supports byte-stream.

I'm working on a version of the patch that asks what the available decoders support, asks for a parser if only one format type is supported, and doesn't ask for it if both formats are supported. I think that would cover the general case.

[ntrrgc]

The parser at AppendPipeline level "might" not be needed (I removed it and the couple of MSE tests I checked still pass)

The parsers in AppendPipeline have the function of creating metadata that may be missing in the container. There are many examples of this problem here and there. Notably, some MPEG TS to MSE ISO BMFF bytestream transmuxers don't mark the "sync-sample" bit for any frames in the container. This is the case with the infamous racecar test stream. Without h264parse filling that information by parsing the NAL units, the browser can't know what frames are sync frames and therefore seeking or performing quality changes will cause video artifacts.

[eocanha]

Please, @emutavchi and @ntrrgc, have a look at the different implementation in 842f29e from branch https://github.com/WebPlatformForEmbedded/WPEWebKit/tree/eocanha/pr948.

@emutavchi, would that way of relaxing the presence of the parsers work for you? It works on the Pi when h264parse is present and fails when not (as intended), declaring video/x-h264 as unsupported in that case (and breaking lots of YouTube tests in that situation, as expected). The patch is a bit restrictive in the sense that if it finds a hardware decoder that only supports one of stream-format={byte-stream,avc}, it demands a parser to be present (even if there's a software decoder supporting all the formats!) in order to declare h264 as supported.

In your specific system (Nexus), I'm assuming that your hardware decoder will support both byte-stream and avc, so the new code wouldn't demand a parser and would enable h264. The same happens for h265 and byte-stream, hev1 and hvc1. Please, can you check if this implementation would work for you?

@ntrrgc, The patch I linked would allow MSE to work without an h264parse. You're right stating that it might create problems under some circumstances, when a parser would actually be needed to complete some info from the buffers. In those cases, @emutavchi would be assuming the risk. I've added a big warning in the gst logs to tell the developer that not having a parser is a bad idea. Would that be enough for you?

[emutavchi]

Hi @eocanha,

Please, can you check if this implementation would work for you?

sure, I'll give it a try and update it here.

@emutavchi would be assuming the risk. I've added a big warning in the gst logs to tell the developer that not having a parser is a bad idea. Would that be enough for you?

how this should work for protected streams? I mean in cases where a software parser cannot be used at all. should it still throw a warning?

[eocanha]

It wouldn't throw a warning in that case. See this append pipeline dump from https://ytlr-cert.appspot.com/2019/main.html?test_type=encryptedmedia-test&tests=33&command=run:

In these kind of cases, the caps are application/x-cenc, and therefore the code doesn't enter in the video/x-h264 code block that wants to instantiate the parser and prints the warning if not possible. Then, what happens in the playback pipeline depends on what the video decoder supports. If it supports all the stream-format variants, it should work. That's what you have to confirm on your side.

[ntrrgc]

In case anyone is interested, I just made a PR with a documentation patch for the AppendPipeline parsers. https://github.com/WebKit/WebKit/pull/5813/files It goes in detail about the motivations for each parser.

[philn]

Why is parsing not wanted anyway? You mention security but I don't think this argument holds for content encrypted using sub-sample encryption. Is your encrypted content using a different encryption scheme?

[emutavchi]

In these kind of cases, the caps are application/x-cenc, and therefore the code doesn't enter in the video/x-h264 code block that wants to instantiate the parser and prints the warning if not possible.

@eocanha thanks for the explanation. I did try on one platform so far. the change looks good. We have to disable the injection of the optional parsers in AppendPipeline (because of clear-to-encrypted-to-clear transition support ). So, basically, I tested only the change in GStreamerRegistryScanner.cpp. I'll do a smoke test on a few more devices and let you know.

@philn

Why is parsing not wanted anyway?

I'm not saying it is not wanted. if available (and applicable for the stream) it will be injected into the playback pipeline. but, the playback pipeline uses hw parsers though, on the platforms I've tested.

You mention security but I don't think this argument holds for content encrypted using sub-sample encryption. Is your encrypted content using a different encryption scheme?

I don't follow your point... what security are you referring to here?

[ntrrgc]

I see your point.

If the h264parser is not going to be used because all content will be cenc, why should it be mandatory for h264 support?

Still, announcing h264 in this case exposes two potential problems:

• The current code in AppendPipeline assumes that, if playing unencrypted h264, h264parse instantiation won't fail, and later will crash when trying to link an element who is a null pointer. That is not a very graceful failure. Instead, createOptionalParserForFormat() could fall back to an identity element and print a big warning.
• Some content would break without a parser. If the graceful fallback is done, people installing and deploying WebKit could not realize it's in place because things seem to work, until they don't.

I would advice everyone deploying WebKit for multimedia to install the GStreamer parser elements. They're not that big, and they're the kind of thing that you don't need it until you run into an use case where you need them and suddenly can't make sense of why things aren't working.

The patch looks good to me, but I would add big warnings in AppendPipeline for everytime a parser instantiation fails, telling the user they need it, plus gracefully falling back to an identity filter to not crash the AppendPipeline after. With said changes, in your case, as long as you only play encrypted h264, no parser instantiation would be attempted and no warning would be printed. Yet, if for whatever reason you end up playing unencrypted h264 in that platform, the warning will be there.

[philn]

You mention security but I don't think this argument holds for content encrypted using sub-sample encryption. Is your encrypted content using a different encryption scheme?

I don't follow your point... what security are you referring to here?

Quoting the first message in this PR: "Some platforms disable/remove software parsers because of the way encrypted playback is implemented" so i thought you didn't want those parsers because of DRM security level requirements, but maybe i misunderstood.

[emutavchi]

@ntrrgc, thanks for the detailed explanation. the reason why we disable parser injection in AppendPipeline is DAI(Dynamic Ad Insertion). some web apps insert unencrypted content in the middle of encrypted playback. we have a custom change to handle this use case. it disables injection of the parsers and also handles caps change in the playback pipeline and injects/removes decryptor elements on demand. based on your feedback, it sounds like we might need to revisit this and dynamically insert parsers for clear content at least.

@philn

so i thought you didn't want those parsers because of DRM security level requirements, but maybe i misunderstood

I see. I don't know the exact reasoning for disabling software parsers by particular vendors. I suspect that it is indeed triggered by DRM requirements.

Edit: clarification

[emutavchi]

@eocanha I tried your change on devices from 3 different SoC vendors. Smoke testing did not reveal any issues. I guess we can proceed with your change.

[eocanha]

The extended patch is being upstreamed as https://bugs.webkit.org/show_bug.cgi?id=247129 and WebKit/WebKit#5861

[eocanha]

The upstream WebKit/WebKit#5861 landed as WebKit/WebKit@b84521e and has been backported to wpe-2.28 as ebcd12e. Closing this PR.