Bug 214973 – Add support for MediaRecorder bitrate options

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Add support for MediaRecorder bitrate options

[youenn fablet]

Created attachment 405827 [details]
Patch

[youenn fablet]

Created attachment 405847 [details]
Patch

[Eric Carlson]

Comment on attachment 405847 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:70
> +    m_outputBitRate = bitRate;

Shouldn't we limit this to the bitrates we support, or at least log an error if passed open we don't support?

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:196
> +    converterCleanup.release();

Is this manual release necessary?

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:452
> +            // FIXME: Maybe we should error the media recorder if we are not able to get a correct converter.

We should at least log and error, to the JS console if possible.

[youenn fablet]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 405847 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405847&action=review
> 
> > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:70
> > +    m_outputBitRate = bitRate;
> 
> Shouldn't we limit this to the bitrates we support, or at least log an error
> if passed open we don't support?

I added clamping and release logging.
I think we should add JS logs as well but this might be best done when adding the options getters.

> 
> > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:196
> > +    converterCleanup.release();
> 
> Is this manual release necessary?

Yes, I'll rename converterCleanup to make it clearer.

> > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:452
> > +            // FIXME: Maybe we should error the media recorder if we are not able to get a correct converter.
> 
> We should at least log and error, to the JS console if possible.

Difficult as is, as this is platform code but will do when implementing getters.

[youenn fablet]

Created attachment 405915 [details]
Patch

[youenn fablet]

win test failure unrelated.

[youenn fablet]

ping review

[Eric Carlson]

Comment on attachment 405915 [details]
Patch

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

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:102
> +    m_outputBitRate = bitRate;

It seems like we should log here if we don't support bitrate as well as in outputBitRate(), since this is called directly with the value in the options dictionary.

[youenn fablet]

Created attachment 406079 [details]
Patch for landing

[EWS]

Committed r265328: <https://trac.webkit.org/changeset/265328>

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

[Radar WebKit Bug Importer]

<rdar://problem/66627900>

[Truitt Savell]

Reverted r265328 for reason:

Broke 17 MediaRecorder tests.

Committed r265335: <https://trac.webkit.org/changeset/265335>

[youenn fablet]

Created attachment 407102 [details]
Fixing nullptr crashes

[EWS]

commit-queue failed to commit attachment 407102 [details] to WebKit repository.

[EWS]

Committed r266116: <https://trac.webkit.org/changeset/266116>

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