Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use av_fopen_utf8() instead of plain fopen() #26

Closed
wants to merge 3 commits into from

Conversation

softworkz
Copy link
Collaborator

@softworkz softworkz commented May 7, 2022

Unify file access operations by replacing usages of direct calls to posix fopen()

v2: Remove changes to fftools for now
v3: Add some additional replacements
v4: Fix and improve commit messages
v5: Add patch to remap ff_open in libavfilter for MSVC on Windows
v6: Add avfilter/file_open.c to "Files without standard license headers" list
v7: Rebase and use the new avpriv_fopen_utf8() instead

cc: Martin Storsjö martin@martin.st
cc: Soft Works softworkz@hotmail.com
cc: Tobias Rapp t.rapp@noa-archive.com

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented May 7, 2022

Submitted as pull.26.ffstaging.FFmpeg.1651945149.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v1

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v1:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v1

fftools/cmdutils.c Outdated Show resolved Hide resolved
@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented May 8, 2022

User Martin Storsjö <martin@martin.st> has been added to the cc: list.

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented May 9, 2022

User Soft Works <softworkz@hotmail.com> has been added to the cc: list.

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

ffmpeg-codebot bot commented May 9, 2022

Submitted as pull.26.v2.ffstaging.FFmpeg.1652122322889.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v2

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v2:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v2

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Martin Storsjö wrote (reply to this):

On Mon, 9 May 2022, softworkz wrote:

> From: softworkz <softworkz@hotmail.com>
>
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>    use av_fopen_utf8() instead of plain fopen()
>
>    Unify file access operations by replacing usages of direct calls to
>    posix fopen()
>
>    v2: Remove changes to fftools for now
>
> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-26%2Fsoftworkz%2Fsubmit_replace_fopen-v2
> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v2
> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/26
>
> Range-diff vs v1:
>
> 1:  5802c8526c < -:  ---------- fftools: use av_fopen_utf8() instead of plain fopen()
> 2:  3266640a93 = 1:  e47287be64 avfilter: use av_fopen_utf8() instead of plain fopen()
>
>
> libavfilter/af_firequalizer.c | 2 +-
> libavfilter/vf_deshake.c      | 2 +-
> libavfilter/vf_signature.c    | 4 ++--
> libavfilter/vf_ssim.c         | 2 +-
> libavfilter/vf_vmafmotion.c   | 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)

LGTM I think. For fully fixing the situation about this function, I 
believe we're going to need to rename it (as the proper solution won't be 
a public function), but as there's already some uses, it's probably fine 
to first take it into use consistently, and then rename all the 
occurrances later.

But we should probably add a copy of file_open.o in libavfilter too (as 
you noted). This is indeed a preexisting problem, but the issue will 
become more visible if we use it in more places.

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Soft Works wrote (reply to this):



> -----Original Message-----
> From: Martin Storsjö <martin@martin.st>
> Sent: Tuesday, May 10, 2022 10:12 PM
> To: softworkz <ffmpegagent@gmail.com>
> Cc: ffmpeg-devel@ffmpeg.org; Soft Works <softworkz@hotmail.com>
> Subject: Re: [PATCH v2] avfilter: use av_fopen_utf8() instead of plain
> fopen()
> 
> On Mon, 9 May 2022, softworkz wrote:
> 
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >    use av_fopen_utf8() instead of plain fopen()
> >
> >    Unify file access operations by replacing usages of direct calls
> to
> >    posix fopen()
> >
> >    v2: Remove changes to fftools for now
> >
> > Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-
> ffstaging-26%2Fsoftworkz%2Fsubmit_replace_fopen-v2
> > Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-
> ffstaging-26/softworkz/submit_replace_fopen-v2
> > Pull-Request: https://github.com/ffstaging/FFmpeg/pull/26
> >
> > Range-diff vs v1:
> >
> > 1:  5802c8526c < -:  ---------- fftools: use av_fopen_utf8() instead
> of plain fopen()
> > 2:  3266640a93 = 1:  e47287be64 avfilter: use av_fopen_utf8()
> instead of plain fopen()
> >
> >
> > libavfilter/af_firequalizer.c | 2 +-
> > libavfilter/vf_deshake.c      | 2 +-
> > libavfilter/vf_signature.c    | 4 ++--
> > libavfilter/vf_ssim.c         | 2 +-
> > libavfilter/vf_vmafmotion.c   | 2 +-
> > 5 files changed, 6 insertions(+), 6 deletions(-)
> 
> LGTM I think. For fully fixing the situation about this function, I
> believe we're going to need to rename it (as the proper solution won't
> be
> a public function), but as there's already some uses, it's probably
> fine
> to first take it into use consistently, and then rename all the
> occurrances later.

Makes sense. Thanks for reviewing.

> But we should probably add a copy of file_open.o in libavfilter too
> (as
> you noted). This is indeed a preexisting problem, but the issue will
> become more visible if we use it in more places.

Would you able to submit a patch for this or shall I?
(I'd prefer the former ;-)

Thanks,
softworkz
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

Submitted as pull.26.v3.ffstaging.FFmpeg.1652747942.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v3

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v3:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v3

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Tobias Rapp wrote (reply to this):

On 17/05/2022 02:39, ffmpegagent wrote:
> Unify file access operations by replacing usages of direct calls to posix
> fopen()

As the cover letter will be gone after commit I think it would be a good 
idea to add the reason for the change also in the commit messages directly.

Is this in preparation for the long filename support on Windows? Then 
maybe this could be mentioned, too.

> v2: Remove changes to fftools for now
> v3: Add some additional replacements
> 
> softworkz (2):
>    avfilter: use av_fopen_utf8() instead of plain fopen()
>    avfilter/dvdsubdec: use av_fopen_utf8() instead of plain fopen()

Patch #2 title should start with "avcodec/dvdsubdec: ..." I guess.

> 
>   libavcodec/dvdsubdec.c            | 2 +-
>   libavfilter/af_firequalizer.c     | 2 +-
>   libavfilter/vf_deshake.c          | 2 +-
>   libavfilter/vf_psnr.c             | 2 +-
>   libavfilter/vf_signature.c        | 4 ++--
>   libavfilter/vf_ssim.c             | 2 +-
>   libavfilter/vf_vidstabdetect.c    | 2 +-
>   libavfilter/vf_vidstabtransform.c | 2 +-
>   libavfilter/vf_vmafmotion.c       | 2 +-
>   9 files changed, 10 insertions(+), 10 deletions(-)
> 
> [...]

Regards,
Tobias

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@ffmpeg-codebot
Copy link

User Tobias Rapp <t.rapp@noa-archive.com> has been added to the cc: list.

@ffmpeg-codebot
Copy link

There is an issue in commit 4f34284:
Please wrap lines in the body of the commit message between 60 and 72 characters.

@ffmpeg-codebot
Copy link

There is an issue in commit ce55ee8:
Please wrap lines in the body of the commit message between 60 and 72 characters.

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

There is an issue in commit 4f34284:
Please wrap lines in the body of the commit message between 60 and 72 characters.

@ffmpeg-codebot
Copy link

There is an issue in commit ce55ee8:
Please wrap lines in the body of the commit message between 60 and 72 characters.

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Soft Works wrote (reply to this):



> -----Original Message-----
> From: Tobias Rapp <t.rapp@noa-archive.com>
> Sent: Tuesday, May 17, 2022 1:56 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>; ffmpegagent <ffmpegagent@gmail.com>
> Cc: Martin Storsjö <martin@martin.st>; softworkz
> <softworkz@hotmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH v3 0/2] use av_fopen_utf8() instead
> of plain fopen()
> 
> On 17/05/2022 02:39, ffmpegagent wrote:
> > Unify file access operations by replacing usages of direct calls to
> posix
> > fopen()
> 
> As the cover letter will be gone after commit I think it would be a
> good
> idea to add the reason for the change also in the commit messages
> directly.
> 
> Is this in preparation for the long filename support on Windows? Then
> maybe this could be mentioned, too.
> 
> > v2: Remove changes to fftools for now
> > v3: Add some additional replacements
> >
> > softworkz (2):
> >    avfilter: use av_fopen_utf8() instead of plain fopen()
> >    avfilter/dvdsubdec: use av_fopen_utf8() instead of plain fopen()
> 
> Patch #2 title should start with "avcodec/dvdsubdec: ..." I guess.

Yes, yes and yes. ;-)

Thanks a lot for reviewing. Update will follow.

Kind regards,
softworkz
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

Submitted as pull.26.v4.ffstaging.FFmpeg.1652790597.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v4

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v4:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v4

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Tobias Rapp wrote (reply to this):

On 17/05/2022 14:29, ffmpegagent wrote:
> Unify file access operations by replacing usages of direct calls to posix
> fopen()
> 
> v2: Remove changes to fftools for now
> v3: Add some additional replacements
> v4: Fix and improve commit messages
> 
> softworkz (2):
>    avfilter: use av_fopen_utf8() instead of plain fopen()
>    avcodec/dvdsubdec: use av_fopen_utf8() instead of plain fopen()
> 
>   libavcodec/dvdsubdec.c            | 2 +-
>   libavfilter/af_firequalizer.c     | 2 +-
>   libavfilter/vf_deshake.c          | 2 +-
>   libavfilter/vf_psnr.c             | 2 +-
>   libavfilter/vf_signature.c        | 4 ++--
>   libavfilter/vf_ssim.c             | 2 +-
>   libavfilter/vf_vidstabdetect.c    | 2 +-
>   libavfilter/vf_vidstabtransform.c | 2 +-
>   libavfilter/vf_vmafmotion.c       | 2 +-
>   9 files changed, 10 insertions(+), 10 deletions(-)
> 
> [...]

Commit messages look fine to me now. I will leave the decision to others 
about the order of changes -- whether this patch-set comes first, or the 
fix for the CRT linking issue (possibly replacing this public function 
with a private copy).

Regards,
Tobias

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

Submitted as pull.26.v5.ffstaging.FFmpeg.1652996437.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v5

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v5:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v5

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Soft Works wrote (reply to this):



> -----Original Message-----
> From: Tobias Rapp <t.rapp@noa-archive.com>
> Sent: Wednesday, May 18, 2022 9:01 AM
> To: ffmpegagent <ffmpegagent@gmail.com>; ffmpeg-devel@ffmpeg.org
> Cc: Martin Storsjö <martin@martin.st>; Soft Works
> <softworkz@hotmail.com>
> Subject: Re: [PATCH v4 0/2] use av_fopen_utf8() instead of plain
> fopen()
> 
> On 17/05/2022 14:29, ffmpegagent wrote:
> > Unify file access operations by replacing usages of direct calls to
> posix
> > fopen()
> >
> > v2: Remove changes to fftools for now
> > v3: Add some additional replacements
> > v4: Fix and improve commit messages
> >
> > softworkz (2):
> >    avfilter: use av_fopen_utf8() instead of plain fopen()
> >    avcodec/dvdsubdec: use av_fopen_utf8() instead of plain fopen()
> >
> >   libavcodec/dvdsubdec.c            | 2 +-
> >   libavfilter/af_firequalizer.c     | 2 +-
> >   libavfilter/vf_deshake.c          | 2 +-
> >   libavfilter/vf_psnr.c             | 2 +-
> >   libavfilter/vf_signature.c        | 4 ++--
> >   libavfilter/vf_ssim.c             | 2 +-
> >   libavfilter/vf_vidstabdetect.c    | 2 +-
> >   libavfilter/vf_vidstabtransform.c | 2 +-
> >   libavfilter/vf_vmafmotion.c       | 2 +-
> >   9 files changed, 10 insertions(+), 10 deletions(-)
> >
> > [...]
> 
> Commit messages look fine to me now. I will leave the decision to
> others
> about the order of changes -- whether this patch-set comes first, or
> the
> fix for the CRT linking issue (possibly replacing this public function
> with a private copy).

To eliminate the question, I'm including that change in
the v5 of this patchset.

Thanks,
softworkz


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

Submitted as pull.26.v6.ffstaging.FFmpeg.1653002114.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v6

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v6:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v6

Unify file access operations by replacing usages of direct calls
to posix fopen() to prepare for long filename support on Windows.

Signed-off-by: softworkz <softworkz@hotmail.com>
Unify file access operations by replacing usages of direct calls
to posix fopen() to prepare for long filename support on Windows.

Signed-off-by: softworkz <softworkz@hotmail.com>
@softworkz
Copy link
Collaborator Author

/submit

@ffmpeg-codebot
Copy link

Submitted as pull.26.v7.ffstaging.FFmpeg.1653305170.ffmpegagent@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-26/softworkz/submit_replace_fopen-v7

To fetch this version to local tag pr-ffstaging-26/softworkz/submit_replace_fopen-v7:

git fetch --no-tags https://github.com/ffstaging/FFmpeg tag pr-ffstaging-26/softworkz/submit_replace_fopen-v7

@ffmpeg-codebot
Copy link

On the FFmpeg mailing list, Martin Storsjö wrote (reply to this):

On Mon, 23 May 2022, ffmpegagent wrote:

> Unify file access operations by replacing usages of direct calls to posix
> fopen()
>
> v2: Remove changes to fftools for now
> v3: Add some additional replacements
> v4: Fix and improve commit messages
> v5: Add patch to remap ff_open in libavfilter for MSVC on Windows
> v6: Add avfilter/file_open.c to "Files without standard license headers"
> list
> v7: Rebase and use the new avpriv_fopen_utf8() instead
>
> softworkz (2):
>  avfilter: use av_fopen_utf8() instead of plain fopen()
>  avcodec/dvdsubdec: use av_fopen_utf8() instead of plain fopen()

Pushed, with the commit messages updated to use avpriv_fopen_utf8 too.

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

@softworkz softworkz closed this May 25, 2022
@ffmpeg-codebot
Copy link

Invalid author email in b43ed59: "4985349+softworkz@users.noreply.github.com"

@ffmpeg-codebot
Copy link

There is a merge commit in this Pull Request:

b43ed593bcf7585cd0aae27d52c70f6920540cca

Please rebase the branch and force-push.

@softworkz
Copy link
Collaborator Author

Merged upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant