Bug 226219 – Support H264 profiles in MediaRecorder

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Support H264 profiles in MediaRecorder

[youenn fablet]

<rdar://78027944>

[youenn fablet]

Created attachment 429649 [details]
Patch

[youenn fablet]

Created attachment 429652 [details]
Patch

[Eric Carlson]

Comment on attachment 429652 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        By default, use baseline profile, which is not VideoToolbox profile but has wider decoding support.

I'm not sure what you mean by "which is not VideoToolbox profile but has wider decoding support", it sounds like you are saying that VideoToolbox doesn't support baseline.

> Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:52
> +        if (startsWithLettersIgnoringASCIICase(codec, "avc1.") && codec.length() >= 11) {
> +            if (codec[5] == '6' && codec[6] == '4')
> +                profile = Profile::High;
> +            else if (codec[5] == '4' && (codec[6] == 'd' || codec[6] == 'D'))
> +                profile = Profile::Main;
> +            break;

Do we want to use Baseline if the configuration is not supported or invalid? It is probably worth logging in that case whatever we do.

> Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:165
> +    RELEASE_LOG_ERROR_IF(error, MediaStream, "VideoSampleBufferCompressor VTSessionSetProperty kVTCompressionPropertyKey_ProfileLevel failed with %d", error);

Probably with logging what profile failed.

[youenn fablet]

(In reply to Eric Carlson from comment #4)
> Comment on attachment 429652 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429652&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        By default, use baseline profile, which is not VideoToolbox profile but has wider decoding support.
> 
> I'm not sure what you mean by "which is not VideoToolbox profile but has
> wider decoding support", it sounds like you are saying that VideoToolbox
> doesn't support baseline.

It is not VideoToolbox default apparently.

> 
> > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:52
> > +        if (startsWithLettersIgnoringASCIICase(codec, "avc1.") && codec.length() >= 11) {
> > +            if (codec[5] == '6' && codec[6] == '4')
> > +                profile = Profile::High;
> > +            else if (codec[5] == '4' && (codec[6] == 'd' || codec[6] == 'D'))
> > +                profile = Profile::Main;
> > +            break;
> 
> Do we want to use Baseline if the configuration is not supported or invalid?
> It is probably worth logging in that case whatever we do.

I'll check whether we have cases where baseline, main or high are not supported.
If it fails, then we use the OS default, which is not too bad.

> > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:165
> > +    RELEASE_LOG_ERROR_IF(error, MediaStream, "VideoSampleBufferCompressor VTSessionSetProperty kVTCompressionPropertyKey_ProfileLevel failed with %d", error);
> 
> Probably with logging what profile failed.

OK.

[Eric Carlson]

Comment on attachment 429652 [details]
Patch

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

>>> Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:52
>>> +            break;
>> 
>> Do we want to use Baseline if the configuration is not supported or invalid? It is probably worth logging in that case whatever we do.
> 
> I'll check whether we have cases where baseline, main or high are not supported.
> If it fails, then we use the OS default, which is not too bad.

I meant should we use baseline if the specified profile isn't supported or is invalid but now that I look, the spec says we should throw if we don't support the MIME type:

3. If invoking `is type supported` with options’ mimeType member as its argument returns false, throw a NotSupportedError DOMException and abort these steps.

so this isn't correct.

This means the logic should also be used by MediaRecorderProvider::isSupported so a page can find out what we support.

[youenn fablet]

Created attachment 429853 [details]
Patch

[youenn fablet]

I changed the patch to use baseline as a default if mime type profile does not stick.

> This means the logic should also be used by
> MediaRecorderProvider::isSupported so a page can find out what we support.

Right but this logic is in GPUProcess so we do not have it synchronously.
In any case, I believe baseline is supported everywhere.
If other profiles are not supported, we will use baseline which is not too bad.
We can try to improve feature detection as a follow-up patch.

[EWS]

Committed r278158 (238202@main): <https://commits.webkit.org/238202@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429853 [details].